search

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

Point to Slack `#pkg-registration` for manual merges #498

Closed goerz closed 1 year ago

goerz commented 1 year ago

As discussed on Slack earlier today, and in issue #491, manual merges of new packages will usually only happen after someone pings the #pkg-registration channel on Slack. For new package developers who may not be aware of the Julia Slack, this may cause considerable frustration, potentially alienating them from the Julia ecosystem. Until there is some other workflow in place to handle this entirely within GitHub, it seems best to explicitly recommend that people post a message on Slack to get help with the manual merge.

This PR amends the instructions in the "If you do not want to fix the AutoMerge issues..." bot message, asking the author to post to #pgk-registration in addition to commenting on the GitHub issue.

Closes #491

DilumAluthge commented 1 year ago

@goerz Since this is a new feature, can you also bump the minor version number in the Project.toml file?

ericphanson commented 1 year ago

I think this message makes sense for General but not other registries using RegistryCI. Can we make it optional? We’d need to thread through a keyword argument from the public api functions.

I also think we should default to false and then opt-in in the workflow in General or else tag it as breaking, since private registries will want to opt out.

goerz commented 1 year ago

Ah, yes, I was assuming this was only used for the General registry. Making it optional totally makes sense then, but I might need some guidance on how to do that.

goerz commented 1 year ago

Ok, I added a commit that makes the message optional (and off by default), but I'm not sure what to do to switch it on for the General registry. I'm not quite sure where the comment_text_fail function is called from or how exactly it ties into the bot for General.

goerz commented 1 year ago

It looks like the syntax I'm using for the keyword argument fails in Julia < 1.5. I'm assuming I have to change _comment_disclaimer(; point_to_slack) to _comment_disclaimer(; point_to_slack=point_to_slack)?

DilumAluthge commented 1 year ago

This is the public entrypoint to the package: https://github.com/JuliaRegistries/RegistryCI.jl/blob/74c541850b665654864cb2c9fae455b94ef4e385/src/AutoMerge/public.jl#L67

The suggest_onepointzero variable is a good example here. You can watch how it starts at the top-level (https://github.com/JuliaRegistries/RegistryCI.jl/blob/74c541850b665654864cb2c9fae455b94ef4e385/src/AutoMerge/public.jl#L90) and then it is propagated down until it reaches the place where it is used.

DilumAluthge commented 1 year ago

Looks like there's a merge conflict that needs to be resolved. Probably the version line in `Project.toml.

goerz commented 1 year ago

This is the public entrypoint to the package:

https://github.com/JuliaRegistries/RegistryCI.jl/blob/74c541850b665654864cb2c9fae455b94ef4e385/src/AutoMerge/public.jl#L67

The suggest_onepointzero variable is a good example here. You can watch how it starts at the top-level [...] and then it is propagated down until it reaches the place where it is used.

Got it. The latest commit now exposes point_to_slack in the Automerge.run function. If I understand correctly, once this PR is merged, I would have to follow up with a PR to the General registry to add point_to_slack = true to the automerge.yml.

ericphanson commented 1 year ago

bors try

bors[bot] commented 1 year ago

try

Build succeeded:

goerz commented 1 year ago

I can look into adding an integration test. I'll see if I can figure out test/automerge-integration.jl, but if it's not obvious, I might need some guidance…

ericphanson commented 1 year ago

I think it might just be a matter of adding point_to_slack as another option here https://github.com/JuliaRegistries/RegistryCI.jl/blob/388a266e31f7e380db9ac4f3d972beabe08b91ba/test/automerge-integration.jl#L37 and then setting it to true for some of the following variants and setting it to false for some. I don’t think we need a new variant for this.

goerz commented 1 year ago

Ok, I've added it to the integration test

ericphanson commented 1 year ago

I think it still needs to be passed in below, like here https://github.com/JuliaRegistries/RegistryCI.jl/pull/498/files#diff-e3199c147438f7c1bdb5c32ba3730b317681ccc3f777bf1f4881f766c66400b7R226

ericphanson commented 1 year ago

Bors try

goerz commented 1 year ago

Duh… sorry about that! Should be fixed now. I've added the point_to_slack only to the first AutoMerge.run (in the location you were pointing out).

I'm not quite sure what the second withenv farther below does (with the two calls to AutoMerge.run), but it doesn't have check_license either, nor does it deal with pass/fail; so I left it alone. It might be worth adding some comments to explain the intent of that test, but that has nothing to do with this PR.

goerz commented 1 year ago

One more quick fix to update the @info "Performing integration tests with settings" … line. Is the Bors try something I can/should trigger, or is that only available to maintainers?

ericphanson commented 1 year ago

Only available to maintainers for security reasons but I can trigger it again. Need to wait for it to finish though otherwise it seems to get confused. Are you happy with it now? Next time I can ask bors to merge which will run integration tests then merge if everything passes.

goerz commented 1 year ago

Yeah, I think it should be ready now

bors[bot] commented 1 year ago

try

Build succeeded:

ericphanson commented 1 year ago

Ok great, thanks for the contribution!

bors merge

bors[bot] commented 1 year ago

Build succeeded:

goerz commented 1 year ago

Thanks @DilumAluthge and @ericphanson for your help on this!

So should I do anything to follow up on this, after the RegistryCI v8.2.0 release goes through? Should I start a PR to the General registry sometime this week to update automerge_staging.yml, or automerge.yml directly?