Adds support for using Danger on PRs #2508

orta · 2017-01-05T15:28:46Z

Summary

Adds Danger.

With initial Danger rules:

Warns on No new un-flowed JS file
Fails build when FB Copyright header is not included

Test plan

This Dangerfile does not have the FB Copyright Header, so once Danger is set up she should fail this build.

Discussion

I based this on looking through a few PRs, will dig through a few more now that I've got something to show
Danger relies on Jest's packages, should/can I make the deps local paths inside yarn? So there's no duplication confusion

Setup

This requires a bit of config by @cpojer - you will need to go through the "creating a bot account" - as this is OSS, it should be a new account with zero privileges to any repos.

After that you'll need to add a token to travis for DANGER_GITHUB_API_TOKEN.

orta · 2017-01-05T16:04:55Z

dangerfile.js

+// ("/href/thing", "name") to "<a href="/href/thing">name</a>"
+const createLink = (href: string, text: string): string => 
+  `<a href='${href}'>${text}</a>`;
+


some of these ^ are functions from Danger RB, in the future I should probably move them into DangerJS too - the use case seems pretty universal

yeah please do!

orta · 2017-01-05T16:05:33Z

I think these CI fails are from our usage of jest-transform in Danger, so, this question becomes more important

Danger relies on Jest's packages, should/can I make the deps local paths inside yarn? So there's no duplication confusion

rogeliog · 2017-01-05T15:49:20Z

dangerfile.js

+
+// ["1", "2", "3"] to "1, 2 and 3"
+const toSentence = (array: Array<string>) : string => 
+  array.slice(0, array.length - 1).join(', ') + ' and ' + array.pop();


Is there a case where the array has a length of 1? In that case this will return and 1

oh yeah, definitely! Good catch.

…NPM package

cpojer · 2017-01-05T16:43:16Z

dangerfile.js

+// New JS files should have the FB copyright header
+const noFBCopyrightFiles = newJsFiles.filter(filepath => {
+  const content = fs.readFileSync(filepath).toString();
+  return !content.includes('Facebook, Inc. All rights reserved');


could we make this more complete and check that the first thing in the file is the full FB copyright header?

Sure, might need to be a regex ( the year is variable ) this was the constant part, I can build on this

cpojer · 2017-01-05T16:45:24Z

package.json

@@ -43,7 +45,7 @@
    "postinstall": "node ./scripts/postinstall.js && node ./scripts/build.js",
    "publish": "yarn run build-clean && yarn run build && lerna publish",
    "test": "yarn run typecheck && yarn run lint && yarn run build && yarn run jest && yarn run test-examples",
-    "test-ci": "yarn run typecheck && yarn run lint && yarn run build && yarn run jest-coverage -- -i && yarn run test-examples && node scripts/mapCoverage.js && codecov",
+    "test-ci": "yarn run typecheck && yarn run lint && yarn run build && yarn run jest-coverage -- -i && yarn run test-examples && node scripts/mapCoverage.js && codecov && yarn run danger",


I kinda wish danger could replace codecov.

I assume codecov reads some pre-existing files to derive output? It's totally feasible ( we have ruby Danger parsing JUnit files in our iOS app )

However, as you noted below, it's the state that doesn't necessarily work. In ruby Danger, there is a plugin for persistence (https://github.com/dbgrandi/danger-mcbrain) which could get replicated as a free heroku mongo somewhere or something to keep state

cpojer · 2017-01-05T16:46:31Z

I'll get the access data for the jest-bot and will report back when I added the token.

A few things that would be nice (I will add more ideas some time):

Fails if there are merge commits in the PR.
Fails if there are changes to package.json without changes to yarn.lock.

Does Danger have any form of state? Can we potentially use it to compare the performance of the test run to master?

orta · 2017-01-05T18:02:09Z

Added a stricter rule check for the copyright header
Added the package.json to yarn.lock check, but I made it warn, not fail, a version bump or changes to the scripts would trigger a fail.
Turns out danger needs support for reading the commits, they're not in the raw diff, which makes sense considering you need an API for commit comments etc [DSL] Add support for pulling in all the commits for a PR danger/danger-js#81

cpojer · 2017-01-05T19:24:57Z

@orta I added the token to travis using jest-bot. Is there anything else I need to do?

orta · 2017-01-05T20:21:13Z

The master CI is broken, I'm gonna test without running ESLinter et al

orta · 2017-01-05T21:31:20Z

@cpojer is it possible that you didn't give the github account access token the public scope? I'm getting a 403 when I use the one in travis, but it works fine with a random Artsy OSS one

orta · 2017-01-05T21:33:50Z

dangerfile.js

@@ -0,0 +1,72 @@
+// @flow
+
+// const { danger, fail, warn } = require('danger');


Waiting on a release with danger/danger-js#82 to support having this uncommented ( Danger normally comments out these requires and injects the deps in at runtime. It's only there for tooling support )

cpojer · 2017-01-05T21:52:22Z

I set the "Display value in build log" to "on". Is there something else I need to do?

orta · 2017-01-05T22:38:04Z

I mean at this point:

Everything else seems 👍

cpojer · 2017-01-06T09:31:28Z

done!

jest-bot · 2017-01-06T13:43:59Z

	Fails
🚫	New JS files do not have the Facebook copyright header: dangerfile.js

	Warnings
⚠️	Please ensure that `@flow` and `'use strict'` are enabled on: dangerfile.js

Generated by 🚫 dangerJS

orta · 2017-01-06T15:38:15Z

Rock, that looks good to me. Will see if I can get enough time today to ship the commits API, otherwise might look over the weekend for that 👍

cpojer · 2017-01-06T16:59:25Z

Awesome! Let me know when it's ready and doesn't spam people so much any more ;)

rogeliog · 2017-01-06T17:36:36Z

@orta the Flow link warning has a link to @flow => https://github.com/flow.... should it be in backticks instead? or should it link to the Flow website?

This reverts commit 2b81d21.

) * Cleanup travis install. We don’t need the latest npm3 any longer. * Revert "Adds support for using Danger on PRs (#2508)" This reverts commit 2b81d21.

Taken from jestjs/jest#2508 This would have applied to #101

Based on jestjs/jest#2508 This would have applied to #101

* Initial Dangerfile, adds a rule for including flow and the FB header * [Danger] Use a local path for jest-runtime instead of the downloaded NPM package * [Danger] Makes the FB copyright header check correct * just run danger * Update Danger

…stjs#2525) * Cleanup travis install. We don’t need the latest npm3 any longer. * Revert "Adds support for using Danger on PRs (jestjs#2508)" This reverts commit 2b81d21.

* Initial Dangerfile, adds a rule for including flow and the FB header * [Danger] Use a local path for jest-runtime instead of the downloaded NPM package * [Danger] Makes the FB copyright header check correct * just run danger * Update Danger

…stjs#2525) * Cleanup travis install. We don’t need the latest npm3 any longer. * Revert "Adds support for using Danger on PRs (jestjs#2508)" This reverts commit 2b81d21.

github-actions · 2021-05-14T03:14:50Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

orta added 2 commits January 5, 2017 10:22

Initial Dangerfile, adds a rule for including flow and the FB header

b1a8c56

Merge branch 'master' of git://github.com/facebook/jest into danger

01cbe32

facebook-github-bot added the CLA Signed ✔️ label Jan 5, 2017

orta commented Jan 5, 2017

View reviewed changes

rogeliog reviewed Jan 5, 2017

View reviewed changes

[Danger] Use a local path for jest-runtime instead of the downloaded …

c25926a

…NPM package

orta force-pushed the danger branch from 3eece47 to c25926a Compare January 5, 2017 16:37

cpojer reviewed Jan 5, 2017

View reviewed changes

orta mentioned this pull request Jan 5, 2017

[DSL] Migrate in some generic helper functions danger/danger-js#80

Closed

[Danger] Makes the FB copyright header check correct

a1405cc

Merge branch 'master' of git://github.com/facebook/jest into danger

41781fe

orta force-pushed the danger branch 2 times, most recently from 069bdb9 to c7ede40 Compare January 5, 2017 21:19

orta commented Jan 5, 2017

View reviewed changes

orta force-pushed the danger branch from c7ede40 to 4ec5c58 Compare January 6, 2017 13:42

just run danger

2cae131

orta force-pushed the danger branch from 4ec5c58 to 2cae131 Compare January 6, 2017 18:14

Merge branch 'master' of git://github.com/facebook/jest into danger

043eb6c

orta force-pushed the danger branch from 0070156 to 043eb6c Compare January 6, 2017 18:31

Update Danger

d70208e

orta force-pushed the danger branch from 048c4e3 to a81559b Compare January 6, 2017 20:15

orta mentioned this pull request Jan 6, 2017

Got DangerJS running on Jest orta/moments#1

Open

Merge branch 'master' of git://github.com/facebook/jest into danger

4c67f40

orta force-pushed the danger branch from a81559b to 4c67f40 Compare January 6, 2017 20:44

cpojer merged commit 2b81d21 into jestjs:master Jan 7, 2017

cpojer added a commit to cpojer/jest that referenced this pull request Jan 7, 2017

Revert "Adds support for using Danger on PRs (jestjs#2508)"

833dc59

This reverts commit 2b81d21.

macklinu added a commit to danger/danger-js that referenced this pull request Jan 17, 2017

Warn if changes to package.json and not yarn.lock

96242d8

Taken from jestjs/jest#2508 This would have applied to #101

macklinu mentioned this pull request Jan 17, 2017

Warn if changes to package.json and not yarn.lock danger/danger-js#102

Merged

macklinu added a commit to danger/danger-js that referenced this pull request Jan 17, 2017

Warn if changes to package.json and not yarn.lock

c06c20c

Based on jestjs/jest#2508 This would have applied to #101

github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for using Danger on PRs #2508

Adds support for using Danger on PRs #2508

orta commented Jan 5, 2017 •

edited

Loading

orta Jan 5, 2017

cpojer Jan 5, 2017

orta commented Jan 5, 2017

rogeliog Jan 5, 2017

orta Jan 5, 2017

cpojer Jan 5, 2017

orta Jan 5, 2017

cpojer Jan 5, 2017

orta Jan 5, 2017

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

orta commented Jan 5, 2017

orta Jan 5, 2017

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

cpojer commented Jan 6, 2017

jest-bot commented Jan 6, 2017 •

edited

Loading

orta commented Jan 6, 2017

cpojer commented Jan 6, 2017

rogeliog commented Jan 6, 2017

github-actions bot commented May 14, 2021

		@@ -0,0 +1,72 @@
		// @flow

		// const { danger, fail, warn } = require('danger');

Adds support for using Danger on PRs #2508

Adds support for using Danger on PRs #2508

Conversation

orta commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

orta commented Jan 5, 2017

Choose a reason for hiding this comment

cpojer commented Jan 5, 2017

orta commented Jan 5, 2017

cpojer commented Jan 6, 2017

jest-bot commented Jan 6, 2017 • edited Loading

orta commented Jan 6, 2017

cpojer commented Jan 6, 2017

rogeliog commented Jan 6, 2017

github-actions bot commented May 14, 2021

orta commented Jan 5, 2017 •

edited

Loading

jest-bot commented Jan 6, 2017 •

edited

Loading