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 to start automating some code review tasks #1251

Merged
merged 5 commits into from
Jun 13, 2017

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 11, 2017

Issue: N/A

What I did

How to test

Modify this PR and wait for messages from @storybookbot

@shilman shilman added ci: do not merge in progress maintenance User-facing maintenance tasks labels Jun 11, 2017
@storybookbot
Copy link
Collaborator

storybookbot commented Jun 11, 2017

Fails
🚫

PR is not labeled with one of: ["cleanup","breaking","feature","bug","documentation","maintenance","greenkeeper","other"]

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Jun 11, 2017

Codecov Report

Merging #1251 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1251    +/-   ##
========================================
  Coverage   13.73%   13.73%            
========================================
  Files         207      207            
  Lines        4638     4638            
  Branches      518      648   +130     
========================================
  Hits          637      637            
+ Misses       3545     3448    -97     
- Partials      456      553    +97
Impacted Files Coverage Δ
app/react/src/client/preview/reducer.js 0% <0%> (ø) ⬆️
addons/storyshots/src/test-bodies.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/left_panel.js 20% <0%> (ø) ⬆️
app/react/src/client/preview/init.js 0% <0%> (ø) ⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bfdc25...3682655. Read the comment docs.

@shilman shilman removed ci: do not merge maintenance User-facing maintenance tasks labels Jun 11, 2017
@shilman shilman requested review from ndelangen and tmeasday June 11, 2017 14:27
const foundLabels = intersection(requiredLabels, labels);
if (isEmpty(foundLabels)) {
fail(`PR is not labeled with one of: ${JSON.stringify(requiredLabels)}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense, but only if you consider that an org contributor can set labels - so an OSS contribution wouldn't be able to do this - so it might be worth changing it to warn ( or to check whether the PR author is in the storybooks org )

@ndelangen
Copy link
Member

https://travis-ci.org/storybooks/storybook/builds/241739179#L1335

The command "npm run danger" exited with 1.

@ndelangen
Copy link
Member

I don't think the CI should fail on this? Shouldn't danger have it's own status?

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Jun 11, 2017
@orta
Copy link
Member

orta commented Jun 11, 2017

This run was specifically set to fail if specific labels were missed - so an exit code of 1 is OK, I think you can give Danger access to set a CI status independently of write access to a repo, which should make it show as one of the checks above

@shilman shilman removed the maintenance User-facing maintenance tasks label Jun 11, 2017
@shilman
Copy link
Member Author

shilman commented Jun 11, 2017

@orta I agree with @ndelangen that it should just show up as a failing check like any of the other services above (e.g. codefactor, codecov, etc.)

I have just expanded the token access to commit status now, but danger still exits with a code of 1, breaking the travis build. Should this work?

edit_personal_access_token

@orta
Copy link
Member

orta commented Jun 11, 2017

I think so, I know the ruby version does this - unsure if the JS version does

@shilman
Copy link
Member Author

shilman commented Jun 12, 2017

@orta Looks like it does not, so I filed this issue: danger/danger-js#279

I'll try to take a whack at it at some point. In the meantime, I've converted it to a warning so that it does not break the build.

@shilman shilman added the cleanup Minor cleanup style change that won't show up in release changelog label Jun 13, 2017
@shilman shilman merged commit 36d7ec2 into master Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Minor cleanup style change that won't show up in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants