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

Automate the r=me idiom #39

Open
Centril opened this issue May 28, 2019 · 6 comments · May be fixed by #120
Open

Automate the r=me idiom #39

Centril opened this issue May 28, 2019 · 6 comments · May be fixed by #120

Comments

@Centril
Copy link
Contributor

Centril commented May 28, 2019

Oftentimes reviewers will write something like r=me with green travis.
It would be good to automate this sort of thing such that a reviewer can write:

@bors r=me

and then bors will r+ once travis is actually green.
That way, you don't need any other reviewer to intervene or to delegate bors privileges.

@kennytm
Copy link
Member

kennytm commented May 28, 2019

r=me is explicitly blacklisted because it could also mean r=me after you have fixed this and that. I do not want to change that.

We could add a different opt-in command for like @bors r+ when-green (which could also allow @bors r=someone-else when-green).

We could add a forced pre-check after r+ before testing the PR:

  • if the status+check is all green, homu will test it
  • if the status+check has at least one red, homu will r- the PR (like a merge conflict)
  • if the status+check has at least one yellow, homu will skip for the next PR; if the queue is exhausted homu will check again in 5 minutes (or after a GitHub event is received or other retry mechanism; whichever easier).

The reviewer can opt-out the Travis PR check by @bors trust-me-it-really-works.

One difficulty with automatic r+ is that a CI vendor may spuriously unable to update the GitHub status, keeping the PR forever in yellow.

Also, it's hard to verify the correctness without a test framework 🤷‍♀️

@Centril
Copy link
Contributor Author

Centril commented May 28, 2019

We could add a different opt-in command for like @bors r+ when-green (which could also allow @bors r=someone-else when-green).

Sure, that sounds good. The actual command isn't that important to me.

We could add a forced pre-check after r+ before testing the PR:

Maybe... that seems like a more invasive change that might be annoying when doing rollup ops or when accepting obviously passing PRs. I think when-green is good enough for now. :)

@camelid
Copy link
Member

camelid commented Oct 30, 2020

We talked about this again recently. @tmandry suggested @bors r+ await, which I think would be a good name for this.

@camelid camelid linked a pull request Nov 5, 2020 that will close this issue
@camsteffen
Copy link

camsteffen commented Jan 11, 2022

There's a possibility that fixing CI leads to "oh this requires a lot more changes". So a final explicit approval should always be required IMO. r=me works well as "I trust you to approve given that additional changes are minor".

@camelid
Copy link
Member

camelid commented Jan 11, 2022

My idea of how this feature (@bors r+ await) would work is it would only approve the PR if CI passed. If CI failed, it would cancel the approval and the PR would have to be approved manually.

@camsteffen
Copy link

That makes more sense. Or it could be default behavior and opt-out with @bors r+ now.

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 a pull request may close this issue.

4 participants