Skip to content
This repository has been archived by the owner on Jun 19, 2018. It is now read-only.

Initial implementation of DangerCI #28

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Initial implementation of DangerCI #28

merged 1 commit into from
Apr 12, 2018

Conversation

DrewML
Copy link
Contributor

@DrewML DrewML commented Apr 10, 2018

Previews
image

image

@DrewML DrewML force-pushed the danger branch 29 times, most recently from e29dd57 to 80f257a Compare April 11, 2018 19:23
@DrewML DrewML requested a review from jimbo April 11, 2018 19:26
@DrewML DrewML requested a review from zetlen April 11, 2018 19:26
@DrewML DrewML changed the title (WIP) Do not merge: Start implementing DangerCI Initial implementation of DangerCI Apr 11, 2018
);
// prettier-ignore
const failSummary = failedTests.map(t =>
`<details>
Copy link
Contributor Author

@DrewML DrewML Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly right now, but couldn't use dedent like I normally do because of this bug: danger/danger-js#557.

Will open a PR later

"clean": "rimraf dist",
"lint": "eslint 'src/**/*.js'",
"prettier": "prettier --write '{src/**/,scripts/**/,}*.js'",
"prettier:check": "prettier-check '{src/**/,scripts/**/,}*.js'",
"prettier:check": "prettier --list-different '{src/**/,scripts/**/,}*.js'",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were using prettier-check to make it exit with code 1, but now that we're doing it in danger we don't need that anymore.

"enzyme": "^3.2.0",
"enzyme-adapter-react-16": "^1.1.0",
"eslint": "^4.12.1",
"eslint-plugin-jsx-a11y": "^6.0.2",
"eslint-plugin-react": "^7.5.1",
"execa": "^0.10.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because child_process.exec doesn't put the local bin in node_modules in the path by default, and execa handles the windows compat problems


general:
artifacts:
- "coverage/lcov-report"

dependencies:
Copy link
Contributor Author

@DrewML DrewML Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CircleCI does not allow you to selectivity expose sensitive environment variables to certain branches only. So for security reasons, I had to disable auto deployment to npm, and I removed our token from Circle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could patch this with a little now deployment of some webhook responder, if it would really help your preferred workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we could do that at some point. Other option would be to migrate to Travis CI, which has support for this.

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple functions for CI. 👍

dangerfile.js Outdated
const { fail } = require('danger');

const fromRoot = path => path.replace(`${process.cwd()}/`, '');
const codeFence = str => `\`\`\`\n${str.trim()}\n\`\`\``;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. ${'```'}\n${str.trim()}\n${'```'}, perhaps?

Maybe even set the backticks to a variable, just for readability. ${fence}\n${str.trim()}\n${fence}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍Updated


function eslintCheck() {
try {
execa.sync('npm', ['run', '--silent', 'lint', '--', '-f', 'json']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this boil down to? npm run --silent lint -- -f json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • npm run --silent prevents npm from spitting any of its own info to stdout (it normally shows the command you ran, and spits info to stderr on failures, which we don't want)

  • -- tells npm to pass all other arguments past that point directly to the command being run

    As of [email protected], you can use custom arguments when executing scripts. The special option -- is used by getopt to delimit the end of the options. npm will pass all the arguments after the -- directly to your script.

  • -f json is shorthand for --format json, which gives us parseable output from the ESLint CLI.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the simplicity of the dangerfile, and it's more than fine as a first implementation. I think we should think about how to parallelize task queues in the future, maybe using Danger's schedule.


general:
artifacts:
- "coverage/lcov-report"

dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could patch this with a little now deployment of some webhook responder, if it would really help your preferred workflow.

@DrewML DrewML merged commit 7945149 into master Apr 12, 2018
@DrewML DrewML deleted the danger branch April 12, 2018 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants