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

[Feature] webpack-bot integration for webpack-cli #220

Closed
wants to merge 7 commits into from
Closed

[Feature] webpack-bot integration for webpack-cli #220

wants to merge 7 commits into from

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Dec 18, 2017

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
No

If relevant, did you update the documentation?
No

Summary
Fixes #195

Does this PR introduce a breaking change?
No

Other information
The configuration is copied from webpack repository and adapted to the current repository. The actions that the bot will do are:

  • Adding labels CI-ok and CI-not-ok to the PRs (labels needed)
  • Reporting error if linter and tests fail
  • Reporting general error
  • Adding label PR: conflict (labels needed)
  • Adding labels unreviewed, reviewed and review-outdated to the PRs (labels needed)
  • Adding label PR: small if the PR contains small changes (label needed)
  • Adds a message if the PR is done from a branch that is master
  • Moves tasks between repos of the same organization
  • Marks inactive issues
  • Checks issues every week

I also added two new npm scripts that will be used just by the bot.

@ematipico ematipico changed the title Added open-bot.yml configuration file for open bot, plus two npm scripts webpack-bot integration for webpack-cli Dec 18, 2017
@ematipico ematipico changed the title webpack-bot integration for webpack-cli [Feature] webpack-bot integration for webpack-cli Dec 18, 2017
@evenstensberg
Copy link
Member

Hey @ematipico , awesome! Any way to test this without merging? I.e make it comment some stuff?

@ematipico
Copy link
Contributor Author

I don't know honestly. It's possible to test locally but when you create a new bot. Here we are adding configuration for a bot that already exists and already works. Maybe @sokra can help us

open-bot.yml Outdated
ensure_1:
value: "{{status_1.state}}"
equals: "failure"
not:
Copy link
Member

Choose a reason for hiding this comment

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

You could omit this not: section, as there is no other CI provider

open-bot.yml Outdated
{{{logResult}}}
```

Instead of updating these (outdated?) unit tests, you can choose to delete them and add integration tests instead. That would be great.
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not relevant here

package.json Outdated
@@ -23,7 +23,9 @@
"precommit": "lint-staged",
"prepublish": "flow-remove-types lib/ -d dist/ && npm run format:dist",
"pretest": "npm run lint",
"test": "jest --coverage"
"test": "jest --coverage",
"travis:unit": "npm run test",
Copy link
Member

Choose a reason for hiding this comment

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

travis:unit -> travis:test as this includes integration and unit test

package.json Outdated
"test": "jest --coverage"
"test": "jest --coverage",
"travis:unit": "npm run test",
"travis:lint": "npm run lint"
Copy link
Member

Choose a reason for hiding this comment

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

You need to adapt .travis.yml too

open-bot.yml Outdated
message: |-
@{{commit.author.login}} The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from [travis]({{status_1.target_url}}) ({{status_1.state}}) and [appveyor]({{status_2.target_url}}) ({{status_2.state}}) and fix these issues.
Copy link
Member

Choose a reason for hiding this comment

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

status_2 is not defined and not needed as there is only one CI provider.

@ematipico
Copy link
Contributor Author

@sokra PR updated with the requested changes

- npm run prepublish
- npm run lint
- npm run test
script: npm run travis:$JOB_PART
Copy link
Member

Choose a reason for hiding this comment

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

JOB_PART is not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the file based on what we already have inside the webpack/webpack repository. I hope we are on the right path

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

You should rebase this against the v2 branch, and make use of the yarn prepare option we have there. Might be some rebasing issues, so you can open in a new PR.

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.

3 participants