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: Add workflow for author approved label #98815

Merged
merged 11 commits into from
Jan 29, 2024

Conversation

ericphanson
Copy link
Member

This intends to facilitate AutoMerge reading this label and bypassing some checks if the author approves. The intended use is:

  1. AutoMerge would have guidelines that can be bypassed if the author approves. Without approval, these fail and the automerge comment instructs the author (well: whoever registered the version/package) to comment with "approved".
  2. If the author does so, that triggers this workflow, which adds the "author-approved" label to the PR
  3. The AutoMerge workflow would be updated to retrigger when this label is applied. It would run through the same logic, but this time notice the label, and use it to bypass those checks.

Currently this workflow is useless as AutoMerge does not have such conditional checks, but I wanted to put a PR to try it out.

Note that JuliaRegistries/RegistryCI.jl#536 shows how easy it is to use label information within automerge itself, which I think makes this a viable route.

For the sake of testing, I will add text to trigger the regex:

@ericphanson
Copy link
Member Author

approved

@ericphanson
Copy link
Member Author

Ah, I can't test this here, since it's an issue_comment triggered workflow:

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

I suppose if we merge this, I can test it in a separate PR that could presumably update the workflow and it would get the changes from the branch instead of from main?

@ericphanson ericphanson marked this pull request as ready for review January 13, 2024 15:00
@DilumAluthge
Copy link
Member

I suppose if we merge this, I can test it in a separate PR that could presumably update the workflow and it would get the changes from the branch instead of from main?

I think, for issue comments and PR comments, it always picks up the workflow file from the default branch :(

We could make a fork of General and do some testing there?

@DilumAluthge
Copy link
Member

I wonder if we could come up with a different name other than "author"? In the AutoMerge codebase, we always use "author" to refer to the actual GitHub PR author, so @JuliaRegistrator, @jlbuild, etc.

So it might be a little confusing to also use the same term "author" for the human that triggered Registrator.

Maybe "registration-trigger-user` or something like that?

@ericphanson
Copy link
Member Author

For the label? I think we should make sure it's understandable by the user, so they can know that their approval was picked up correctly. So I think it shouldn't be too obscure. What about package-author-approved?

@ericphanson
Copy link
Member Author

ericphanson commented Jan 14, 2024

Ok, I've tested it here, and updated this PR.

Mismatch:
Screenshot 2024-01-14 at 23 48 40
Match:
Screenshot 2024-01-14 at 23 48 52

Note the times on the Julia part vary from 1-2s up to 15-20s, I'm not really sure why. However that code only runs when a comment is posted from a non-bot user that includes "approved", so hopefully it doesn't need to run too much.

@DilumAluthge
Copy link
Member

Can we also exclude the github-actions[bot] user?

@ericphanson
Copy link
Member Author

I have excluded all bot users

@DilumAluthge
Copy link
Member

For the label? I think we should make sure it's understandable by the user, so they can know that their approval was picked up correctly. So I think it shouldn't be too obscure. What about package-author-approved?

Yeah let's go with that.

@ericphanson
Copy link
Member Author

I had already renamed the label to package-author-approved. Just now, I changed the approval string to "merge approved", and realized in fact it is not case-sensitive, as contains is not. I updated the comment with a reference.

I think this is good to go, and am writing a PR to RegistryCI to make use of this in the sequential version skipping check (probably to the delight of like 2 people who like skipping versions). We can also use it for other things down the line.

.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
.github/workflows/author_approval.yml Outdated Show resolved Hide resolved
@DilumAluthge DilumAluthge changed the title add workflow for author approved label CI: Add workflow for author approved label Jan 28, 2024
@ericphanson ericphanson merged commit 75343fd into master Jan 29, 2024
10 checks passed
@ericphanson ericphanson deleted the eph/create-author-approved-label branch January 29, 2024 13:17
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.

2 participants