Skip to content

Bring back mandatory travis-ci check #412

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.

Sign up for GitHub

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

Jump to bottom
Merged
chaosbot merged 3 commits into from
Jun 2, 2017
Merged

Bring back mandatory travis-ci check #412

chaosbot merged 3 commits into from
Jun 2, 2017

Conversation

@phil-r
Copy link
Member

@phil-r phil-r commented May 30, 2017 •
edited
Loading

How it works:

  1. Before polling all issues the status of the master is checked
  2. If and only if it's passing CI(Travis) for each open PR its build status will be checked.
  3. From PR object (example for this PR) sha of the head commit is taken and it's status (example for this PR) is checked.
  4. If and only if it's failing PR is marked with ci failed label and skipped.
  5. ???
  6. PROFIT!

TODO:

  • It doesn't block PRs if status is not added for some reason
  • Add new label ci failed for build with failing CI
  • Check current master status - if it fails - do nothing
  • Close PRs that can't pass CI for a long time if master is okay (as we do with PRs with conflicts)
  • Fix and update tests
  • Test on my fork

@andrewda
Copy link
Member

andrewda commented May 31, 2017

I don't think we should close PR's that don't pass CI – or if we do make a very large timeout. Take this example: if the base master branch is failing for whatever reason, a PR to fix the CI would be required to be merged before other PR's would start passing again (because we don't want to encourage people to fix CI issues in unrelated PR's).

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 31, 2017

@andrewda I'd hope that this would prevent Travis from breaking on master, but you're right that it could still happen.

@andrewda
Copy link
Member

andrewda commented May 31, 2017 •
edited
Loading

@PlasmaPower My main worry was that if something like fd6ce27 or 39b7867 were to happen, where a hotfix is pushed without going through a PR.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 31, 2017

Oh yeah, good point.

@mdcfe
Copy link
Contributor

mdcfe commented May 31, 2017 •
edited
Loading

In my opinion, if master is failing, we shouldn't force PRs to pass Travis, but if it is stable, we should add a timeout period to the end.

Edit: if this changes, I'll upvote.

@andrewda andrewda mentioned this pull request May 31, 2017
Merged
@phil-r
Copy link
Member Author

phil-r commented May 31, 2017

Nice ideas guys! I've updated TODO

@mdcfe
Copy link
Contributor

mdcfe commented May 31, 2017

Looks good now! 👍

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 31, 2017

Ironic, this breaks Travis.

PlasmaPower
PlasmaPower suggested changes May 31, 2017
View reviewed changes
Copy link
Contributor

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

I still don't see where it checks if master is failing.

github_api/prs.py Outdated
continue

# check if this PR successfully passed travis-ci
build_passed = has_build_passed(api, pr["statuses_url"])
Copy link
Contributor

@PlasmaPower PlasmaPower May 31, 2017

Choose a reason for hiding this comment

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

You need to pass in the SHA here.

Copy link
Member Author

@phil-r phil-r May 31, 2017

Choose a reason for hiding this comment

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

yeah, didn't have time to finish, will try to do it this evening.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 31, 2017

Oh, sorry, I didn't see that this was WIP.

@phil-r phil-r force-pushed the travis-pr-check branch from e4eda03 to 48cd86a Compare June 1, 2017 01:30
@phil-r
Copy link
Member Author

phil-r commented Jun 1, 2017 •
edited
Loading

@PlasmaPower feel free to review, it's basically finished, but I can't test as I have issues running docker after all updates.
Also I have no idea how I will resolve conflicts with #420 xD

PlasmaPower
PlasmaPower suggested changes Jun 1, 2017
View reviewed changes
github_api/prs.py Outdated
return api("PUT", path, json=data)


def close_stale_pr(api, urn, pr, delta, reason):
Copy link
Contributor

@PlasmaPower PlasmaPower Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe a better name like handle_broken_pr? This implies the PR is stale and the function will close it.

Copy link
Member Author

@phil-r phil-r Jun 1, 2017

Choose a reason for hiding this comment

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

Thanks @PlasmaPower ! I've struggle so much with naming of this function ^_^

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Jun 1, 2017 •
edited
Loading

I checked, and Travis is run for new commits on master: https://github.com/chaosbot/Chaos/commits/master

Edit: also is run for squash commits

I'm assuming you checked that this method of getting the status works?

@phil-r phil-r force-pushed the travis-pr-check branch from 48cd86a to e9ee63f Compare June 1, 2017 19:36
phil-r added 3 commits June 1, 2017 23:28
@phil-r
Bring back mandatory travis-ci check
20031a5 
It doesn't block PRs if status is not added for some reason
It doesn't block them if master is also failing
@phil-r
Register ci failed label
a2a3b08
@phil-r
More bulletproof version
49d954d
@phil-r phil-r force-pushed the travis-pr-check branch from 5925086 to 49d954d Compare June 1, 2017 21:59
@phil-r
Copy link
Member Author

phil-r commented Jun 1, 2017

@PlasmaPower please review ;)

@phil-r phil-r changed the title [WIP] Bring back mandatory travis-ci check Bring back mandatory travis-ci check Jun 1, 2017
@phil-r phil-r closed this Jun 1, 2017
@phil-r
Copy link
Member Author

phil-r commented Jun 1, 2017

🤦‍♂️ I've accidentally closed it

@phil-r phil-r reopened this Jun 1, 2017
@chaosbot chaosbot merged commit 6c56cc4 into Chaosthebot:master Jun 2, 2017
@chaosbot
Copy link
Collaborator

chaosbot commented Jun 2, 2017

🙆‍♀️ PR passed with a vote of 20 for and 0 against, a weighted total of 19.5 and a threshold of 6.5, and a current meritocracy review.

See merge-commit 6c56cc4 for more details.

@phil-r phil-r deleted the travis-pr-check branch June 2, 2017 07:24
@while-loop
Copy link
Contributor

while-loop commented Jun 2, 2017

looks nice =D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewda andrewda andrewda approved these changes

@syvb syvb syvb approved these changes

+1 more reviewer

@PlasmaPower PlasmaPower PlasmaPower approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@phil-r @andrewda @PlasmaPower @mdcfe @chaosbot @while-loop @syvb