Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add danger #15

Merged
merged 1 commit into from
Feb 9, 2018
Merged

Add danger #15

merged 1 commit into from
Feb 9, 2018

Conversation

orta
Copy link
Member

@orta orta commented Nov 13, 2017

Adds:

  • Danger - fixes Add Danger #7
  • A rule to check that a PR has a body
  • A rule to check that changes to rules/* includes a CHANGELOG entry
  • A CHANGELOG - fixes Add a changelog #3

screen shot 2017-11-13 at 8 10 22 am

@orta
Copy link
Member Author

orta commented Nov 13, 2017

Hrnm, interesting - looks like one of Danger's dependencies is a node 6 > dep. So I've made danger get added at runtime.

@SimenB
Copy link
Member

SimenB commented Nov 15, 2017

Woo!

Still failing CI because of node 6.

Would using build stages allow us to only run danger on one env (e.g. node 8)? https://docs.travis-ci.com/user/build-stages

CHANGELOG.md Outdated
@@ -0,0 +1,7 @@
// Please add your own contribution below inside the Master section // These
docs are aimed at users rather than danger developers, so please limit technical
Copy link
Member

Choose a reason for hiding this comment

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

"danger" should probably not be here

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree 👍

@SimenB
Copy link
Member

SimenB commented Nov 25, 2017

I've added a separate changelog, causing a conflict.

(I still want to use conventional commits and just generate it, but now it's there, at least)

@SimenB
Copy link
Member

SimenB commented Dec 5, 2017

I've now deleted the changelog as semantic-release is setup. I'd still love to have the test report from jest and lint report from eslint inline. Danger supports that, right?

Not sure how to deal with node 4. Keen on dropping it, it messes up for semantic release as well :P

@orta orta changed the title Add danger + a CHANGELOG Add danger Dec 14, 2017
@orta
Copy link
Member Author

orta commented Dec 14, 2017

Hah - sorry for letting this drop for so long, still mid-way on a big deadline, but I've switched it up to have danger only run on 8 and to remove the CHANGELOG rule. Right now it only has one simple "all PRs must have a description" rule. Can expand later 👍

@orta
Copy link
Member Author

orta commented Dec 14, 2017

OK - that's that 👍

.travis.yml Outdated
node_js: 8
os: linux
script:
- yarn add -D danger@^2
Copy link
Member

Choose a reason for hiding this comment

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

npx danger@^2?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpojer where is my yarn exec danger ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree though @SimenB

Copy link
Member

Choose a reason for hiding this comment

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

dangerfile.js Outdated

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

// Ensure that people include a description on their PRss
Copy link
Member

Choose a reason for hiding this comment

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

PRss typo?

@orta
Copy link
Member Author

orta commented Dec 14, 2017

Nice, updated 👍

@SimenB
Copy link
Member

SimenB commented Dec 14, 2017

$ npx danger@^2
npx: installed 110 in 4.601s
child process exited with code 0
Request failed [404]: https://api.github.com/repos/jest-community/eslint-plugin-jest/statuses/f66f69562d184115afb2865338910b717be15d1f
Response: {
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3/repos/statuses/#create-a-status"
}
No issues or messages were sent. Removing any existing messages.

Are we supposed to get the 404?

@SimenB
Copy link
Member

SimenB commented Jan 23, 2018

We've added ignore-engines, so you should be able to add danger as a dep now.

@orta
Copy link
Member Author

orta commented Jan 23, 2018

Lols, I should wrap up this PR, yep

@macklinu
Copy link
Collaborator

macklinu commented Feb 9, 2018

@orta let me know if I can help out with this one! 👋

@orta
Copy link
Member Author

orta commented Feb 9, 2018

Ah yeah, the CI fail is nothing to do with danger

@orta orta force-pushed the add_danger branch 2 times, most recently from f66f695 to aad7cfb Compare February 9, 2018 18:53
@orta
Copy link
Member Author

orta commented Feb 9, 2018

Rebased 👍

@orta
Copy link
Member Author

orta commented Feb 9, 2018

The trick was the ordering of the tasks in the travis.yml 👍

@orta orta merged commit 5ea5c23 into master Feb 9, 2018
@SimenB SimenB deleted the add_danger branch February 10, 2018 12:07
@SimenB
Copy link
Member

SimenB commented Feb 11, 2018

Does this matter?

You may have updated from Danger 2.x -> 3.x without updating from danger to danger ci.

https://travis-ci.org/jest-community/eslint-plugin-jest/jobs/340160669#L471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Danger Add a changelog
3 participants