AutoMerge: Allow skipping sequential versions with author-approval criteria #539

ericphanson · 2024-01-27T21:32:55Z

Based on #536 to avoid conflicts. That one should be merged first.

Requires JuliaRegistries/General#98815

The flow here is:

automerge comments, notes they can override
package author comments "[merge approved]"
GHA adds a label
new label triggers automerge again
automerge sees the new label and skips the sequential version check

FAQ:

What happens if some one else comments "merge approved"? The label doesn't get applied, so nothing happens
What happens if someone adds the label manually? The label does take effect. I.e. triage-level maintainers can facilitate author approval if needed. We need to trust them to not abuse this (I doubt it will happen and we can revoke triage access if folks aren't behaving)
Does a "merge approved" comment block automerge unless it contains [noblock]? No, we special case it so comments that trigger author-approval count as noblock.
Does AutoMerge's comment mentioning "merge approved" trigger the labelling workflow? No, it shouldn't, it is a bot user. (If it did, the label would still not be applied, since the registrator is not the package author)
What happens if multiple guidelines use the author-approval mechanism, does the author have to indicate what they are approving? No, merge approved will apply to all of them
What happens if the author comments before even seeing the automerge comment? They would end up approving whatever it says, but only 1 guideline is overridable at this point.
What happens if the PR is from gitlab? We won't be able to find the author's github username in the PR text from Registrator, so the labeling workflow won't be able to check the commenter is the author, so the label won't get added. Triage-level maintainers can manually add the label in this case, but there isn't an automatic solution.
What happens if the PR is not from Registrator? The label could still get added by the workflow (if it hits the regex to have the author name, and the person with that username comments), but automerge won't run, so the label won't do anything.

…nism

DilumAluthge · 2024-01-28T14:19:11Z

GHA adds a label

new label triggers automerge again

This might be a problem. If the label was added using the default GITHUB_TOKEN token, then IIRC the "label-added" event won't trigger any further GitHub Actions.

DilumAluthge · 2024-01-28T14:31:16Z

GHA adds a label

new label triggers automerge again

This might be a problem. If the label was added using the default GITHUB_TOKEN token, then IIRC the "label-added" event won't trigger any further GitHub Actions.

Can we make the "GHA adds a label" step use the custom @JuliaTagBot PAT we have? I believe that this would bypass the above issue.

Of course, maybe we should first verify that the above actually would be an issue.

DilumAluthge · 2024-01-28T14:32:45Z

~~Perhaps we should make the text specific to the guideline being approved? How about something like /approve-merge-skip-version?~~

~~Also, how about we add the / to the front, to emphasize that this is kind of a "command" that the package author is providing?~~

We've settled on [merge approved]

DilumAluthge · 2024-01-28T14:33:36Z

Even if we only get this working for GitHub-hosted packages at first, this still covers the majority of packages, so I still think it'll help reduce the manual registry-committer burden.

ericphanson · 2024-01-28T14:45:16Z

This might be a problem. If the label was added using the default GITHUB_TOKEN token, then IIRC the "label-added" event won't trigger any further GitHub Actions.

Good point, I think that sounds right.

Can we make the "GHA adds a label" step use the custom @JuliaTagBot PAT we have? I believe that this would bypass the above issue.
Of course, maybe we should first verify that the above actually would be an issue.

Yeah, that makes sense. Since this can be done on the GHA by itself, IMO we can work on that once we merge down the stuff here and get RegistryCI updated on General. Then we can try it out and get the authentication working there.

Perhaps we should make the text specific to the guideline being approved? How about something like /approve-merge-skip-version?
Also, how about we add the / to the front, to emphasize that this is kind of a "command" that the package author is providing?

To me this seems overly complicated. Since RegistryCI prints all of the problems it found at once, the author can either approve merging or not knowing that information. I don't really feel like there's more than 1 decision the author actually has, since not approving 1 thing blocks merging (so ultimately they need to decide if they want to merge now or not).

I also think this will lead to more questions/issues where folks approve 1 thing but not another and are confused.

I think "merge approved" is straightforward and simpler.

Even if we only get this working for GitHub-hosted packages at first, this still covers the majority of packages, so I still think it'll help reduce the manual registry-committer burden.

Agreed!

ericphanson · 2024-01-28T14:48:16Z

Re- the token, the docs here seem clear that indeed GITHUB_TOKEN is insufficient for this, and we should use a PAT. We could use an existing one or make a new one for the task.

DilumAluthge · 2024-01-28T14:51:39Z

We could use an existing one or make a new one for the task.

For now, probably easier to re-use the existing one (the @JuliaTagBot token that AutoMerge uses when merging a PR). But we might consider making a new one in the future.

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

example_github_workflow_files/automerge.yml

DilumAluthge · 2024-01-28T23:57:31Z

src/AutoMerge/cron.jl

-pr_comment_is_blocking(c::github.mirror.nvdadr.comment) = !occursin("[noblock]", body(c))
+function pr_comment_is_blocking(c::github.mirror.nvdadr.comment)
+    # Note: `merge_approved` is not case sensitive, to match the semantics of `contains` on GHA
+    not_blocking = occursin("[noblock]", body(c)) || occursin("merge approved", lowercase(body(c)))


I'm not a fan of this special casing. I'd rather just tell users to always include [noblock] in all of their comments.

As discussed in JuliaRegistries/General#98815, we're going to compromise, and we'll require [merge approved], but not require [noblock].

src/AutoMerge/guidelines.jl

src/AutoMerge/util.jl

test/automerge-unit.jl

src/AutoMerge/guidelines.jl

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

test/automerge-unit.jl

docs/src/guidelines.md

src/AutoMerge/cron.jl

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

DilumAluthge · 2024-01-29T13:30:29Z

@ericphanson This needs a rebase, I think.

ericphanson · 2024-01-29T17:42:07Z

(updated)

example_github_workflow_files/automerge.yml

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

DilumAluthge

After these are done, I think this should be good to go.

docs/src/guidelines.md

src/AutoMerge/cron.jl

src/AutoMerge/guidelines.jl

test/automerge-unit.jl

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

DilumAluthge

LGTM

* Upgrade to Documenter v1 * Update make.jl * Update Project.toml --------- Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

ericphanson added 6 commits December 9, 2023 01:46

respect Override AutoMerge: name similarity is okay labels

487e6bf

update docs & example workflow

58e3a6a

only trigger on the correct label

811b8ae

add kwargs to docs

feb2a61

allow skipping sequential version criteria with author approval mecha…

e0a9236

…nism

update pr_comment_is_blocking

cda2ef7

ericphanson requested a review from DilumAluthge January 27, 2024 21:33

fix docs

e57556d

DilumAluthge mentioned this pull request Jan 28, 2024

CI: Add workflow for author approved label JuliaRegistries/General#98815

Merged

Apply suggestions from code review

caf85a1

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

DilumAluthge requested changes Jan 29, 2024

View reviewed changes

Apply suggestions from code review

6386e93

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

ericphanson commented Jan 29, 2024

View reviewed changes

test/automerge-unit.jl Outdated Show resolved Hide resolved

ericphanson added 2 commits January 29, 2024 01:14

Update test/automerge-unit.jl

86a325d

Merge branch 'eph/respect-labels' into eph/author-approved-skip

f07b828

DilumAluthge added the feature label Jan 29, 2024

DilumAluthge changed the title ~~Allow skipping sequential versions with author-approval criteria~~ AutoMerge: Allow skipping sequential versions with author-approval criteria Jan 29, 2024

DilumAluthge requested changes Jan 29, 2024

View reviewed changes

docs/src/guidelines.md Outdated Show resolved Hide resolved

docs/src/guidelines.md Outdated Show resolved Hide resolved

src/AutoMerge/cron.jl Outdated Show resolved Hide resolved

DilumAluthge self-requested a review January 29, 2024 00:40

Apply suggestions from code review

737e5c2

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

Base automatically changed from eph/respect-labels to master January 29, 2024 13:29

Merge branch 'master' into eph/author-approved-skip

b33d6c1

DilumAluthge requested changes Jan 29, 2024

View reviewed changes

example_github_workflow_files/automerge.yml Outdated Show resolved Hide resolved

Update example_github_workflow_files/automerge.yml

2f383b5

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

ericphanson mentioned this pull request Jan 29, 2024

Bump version for release #542

Merged

ericphanson requested a review from DilumAluthge January 29, 2024 21:26

DilumAluthge requested changes Jan 29, 2024

View reviewed changes

ericphanson and others added 2 commits January 29, 2024 22:31

Apply suggestions from code review

d5fd1fa

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>

Merge branch 'master' into eph/author-approved-skip

30c2ffc

ericphanson requested a review from DilumAluthge January 29, 2024 21:38

DilumAluthge approved these changes Jan 29, 2024

View reviewed changes

ericphanson enabled auto-merge January 29, 2024 21:42

ericphanson added this pull request to the merge queue Jan 29, 2024

ericphanson removed this pull request from the merge queue due to a manual request Jan 29, 2024

ericphanson added this pull request to the merge queue Jan 29, 2024

Merged via the queue into master with commit 01458cf Jan 30, 2024
11 checks passed

ericphanson deleted the eph/author-approved-skip branch January 30, 2024 00:08

ericphanson referenced this pull request Jan 30, 2024

Upgrade to Documenter v1 (#521)

1f73987

* Upgrade to Documenter v1 * Update make.jl * Update Project.toml --------- Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

ericphanson mentioned this pull request Jan 30, 2024

Fix sequential version message #546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AutoMerge: Allow skipping sequential versions with author-approval criteria #539

AutoMerge: Allow skipping sequential versions with author-approval criteria #539

ericphanson commented Jan 27, 2024 •

edited by DilumAluthge

Loading

DilumAluthge commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024 •

edited

Loading

DilumAluthge commented Jan 28, 2024

ericphanson commented Jan 28, 2024

ericphanson commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024

DilumAluthge Jan 28, 2024

DilumAluthge Jan 29, 2024

DilumAluthge commented Jan 29, 2024

ericphanson commented Jan 29, 2024

DilumAluthge left a comment

DilumAluthge left a comment

AutoMerge: Allow skipping sequential versions with author-approval criteria #539

AutoMerge: Allow skipping sequential versions with author-approval criteria #539

Conversation

ericphanson commented Jan 27, 2024 • edited by DilumAluthge Loading

DilumAluthge commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024 • edited Loading

DilumAluthge commented Jan 28, 2024

ericphanson commented Jan 28, 2024

ericphanson commented Jan 28, 2024

DilumAluthge commented Jan 28, 2024

DilumAluthge Jan 28, 2024

Choose a reason for hiding this comment

DilumAluthge Jan 29, 2024

Choose a reason for hiding this comment

DilumAluthge commented Jan 29, 2024

ericphanson commented Jan 29, 2024

DilumAluthge left a comment

Choose a reason for hiding this comment

DilumAluthge left a comment

Choose a reason for hiding this comment

ericphanson commented Jan 27, 2024 •

edited by DilumAluthge

Loading

DilumAluthge commented Jan 28, 2024 •

edited

Loading