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

CI: automatically approve a PR if has auto-approve label #100876

Closed
wants to merge 1 commit into from

Conversation

fee1-dead
Copy link
Member

I have no idea if this would work. But given that a lot of people (including me) would like to have this feature, I took a stab.

The current design is placing a dedicated label on PRs such that, when CI has successfully ran, GitHub Actions will tell bors to r= the current assignee of the PR.

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 22, 2022
@rust-highfive
Copy link
Collaborator

r? @pietroalbini

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2022
@Mark-Simulacrum
Copy link
Member

I would like to see some kind of discussion on why we're interested in this feature / what the goal is. In general, it seems like the wrong place to put it -- if we want bors to wait for PR CI to pass before respecting r+, then adjusting bors seems like a better plan (e.g., rust-lang/homu#67, rust-lang/homu#120).

As-is, I don't think this would work, since the user GitHub uses isn't trusted by bors. Further, it would mean that "write" permissions on this repository are enough to merge PRs into master, which is also not what we want (I think today this is more murky in any case, I'm not sure the extent to which branch protections etc are enough here, but we don't want that to look benign via bors testing things).

@Mark-Simulacrum Mark-Simulacrum self-assigned this Aug 22, 2022
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
   Compiling thiserror v1.0.30
   Compiling yaml-merge-keys v0.4.1
   Compiling expand-yaml-anchors v0.1.0 (/checkout/src/tools/expand-yaml-anchors)
    Finished release [optimized] target(s) in 4.76s
error: .github/workflows/ci.yml is not up to date; please run `x.py run src/tools/expand-yaml-anchors`.
caused by: src/ci/github-actions/ci.yml and .github/workflows/ci.yml are different

@fee1-dead
Copy link
Member Author

I would like to see some kind of discussion on why we're interested in this feature / what the goal is.

Yes. That is what I wanted most when I opened this PR. I believe the problem is that when a commit is pushed or when a PR is opened, a reviewer sees that there are no problems and would like to approve the PR, but the CI is still pending. An example of an eager review is #100336 (comment) - where the PR is approved before CI ran and CI failed afterwards. On the other hand, not approving it adds more work for the reviewer as they would then need to go back to the PR again and approve if they see the green results. (sometimes there is more work if the reviewer forgot that they believed the PR was good, so they have to go back and review again)

In general, it seems like the wrong place to put it

Agreed. I only opened this PR because I am not familiar with homu's infrastructure.

Thank you for pointing me to rust-lang/homu#120! cc @camelid, could we redirect the discussion to there instead?

@Mark-Simulacrum
Copy link
Member

I'll go ahead and close this PR, but feel free to open a thread on #t-infra if you'd like to discuss solutions further.

@fee1-dead fee1-dead deleted the auto-approve branch August 23, 2022 11:13
@camelid
Copy link
Member

camelid commented Aug 25, 2022

@fee1-dead I'd love for someone to pick that PR up again, but unfortunately I won't have time myself and I don't remember much of the context of the code. Let me know if you have questions about it though and I'll try to answer them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants