-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add option to create a merge request for GitLab. #49
Conversation
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
+ Coverage 96.15% 96.55% +0.39%
==========================================
Files 1 1
Lines 234 261 +27
==========================================
+ Hits 225 252 +27
Misses 9 9
Continue to review full report at Codecov.
|
4a07360
to
6ba2200
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @GunnarFarneback!
I just tried and it worked out of the box for me. Our RegistryCI ran fine, I suppose we don't do anything related to AutoMerge, which you mentioned might be impacted.
I just have one comment about being able to tag a specific GitLab user, otherwise looks good to me.
# newlines with HTML `<br>` codes. This works but inhibits the | ||
# markdown rendering of the list, so the result does not look | ||
# great. | ||
description = replace(description, "\n" => "<br>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also use <li>
elements (inside an <ul>
) to have a bulleted list rendering.
description = replace(description, "\n" => "<br>") | |
description = "<ul>"*join(map(x -> "<li>$x</li>", split(description, "\n")))*"</ul>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I eventually opted for a unicode bullet, which I think is good enough.
* Registering package: $(pkg.name) | ||
* Repository: $(repo) | ||
* Version: v$(pkg.version) | ||
* Commit: $(commit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing which Registrator.jl does is tag the GitLab user that triggers the registration, which is quite useful because GitLab will notify that person as the PR progresses (notably if RegistryCI tests fail and the PR needs attending).
Could we add an optional argument to this method (and parent methods) in order to add e.g.
* Created by: @$(user_handle)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I had forgotten to refresh my tab for a while and didn't see the PR got merged 😅
Do you think this would be ok to add? Happy to make a PR since it should be quite straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems slightly much to have an additional argument (to register
) for this but I can see the usefulness, so please go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to just pick up the author from some standard GitLab CI variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thanks!
Created #51
440: Revise the commit regex. r=ericphanson a=GunnarFarneback This revises the commit regex to match ``` r"Commit: ([0-9a-f]+)" ``` but no longer requires that it is a markdown item on a line of its own. The rationale is GunnarFarneback/LocalRegistry.jl#49, which makes use of "git push options" to create a pull request (only effective on GitLab). This provides a very low complexity pull request creation mechanism but has the limitation that it cannot pass newlines. In terms of robustness and input sanitation this change makes no difference to Registrator generated pull requests.In fact it's rather stricter since the code in total only allows the commit hash to be exactly 40 hexadecimal digits. Co-authored-by: Gunnar Farnebäck <gunnar.farneback@inify.com>
This PR adds an option to
register
to automatically create a merge request if the registry is hosted on GitLab.Why GitLab, what about GitHub, Bitbucket, etc? The reason is that GitLab supports creating a merge request with "git push options". This means two things:
This is primarily intended for use in CI pipelines. The general idea is that when the conditions for a registration are met, the pipeline would run
The pipeline would need to have a write deploy key for the registry repository set up or credentials encoded within
REGISTRY_URL
.Then the registry can e.g. be equipped with a CI job running RegistryCI and the merge request will be automatically merged once the CI passes.
There is one major limitation of this approach. Git push options do not support embedded newlines (probably they didn't want to get into the LF vs CRLF mire) and GitLab has no proper workaround for that (see https://gitlab.com/gitlab-org/gitlab/-/issues/241710), meaning that MR descriptions are severely handicapped. In fact to the extent that the AutoMerge part of RegistryCI cannot handle it until JuliaRegistries/RegistryCI.jl#440 has been merged.