JuliaRegistries / RegistryCI.jl

Continuous integration (CI) tools for Julia package registries, including registry consistency testing, automatic merging (automerge) of pull requests, and automatic TagBot triggers
https://juliaregistries.github.io/RegistryCI.jl/stable
Other
31 stars 30 forks source link

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

Closed ericphanson closed 7 months ago

ericphanson commented 7 months ago

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

Requires https://github.com/JuliaRegistries/General/pull/98815

The flow here is:

FAQ:

DilumAluthge commented 7 months ago
  • 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 commented 7 months ago
  • 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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

DilumAluthge commented 7 months ago

@ericphanson This needs a rebase, I think.

ericphanson commented 7 months ago

(updated)