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

Add option to create a merge request for GitLab. #49

Merged
merged 4 commits into from
Feb 13, 2022
Merged

Conversation

GunnarFarneback
Copy link
Owner

@GunnarFarneback GunnarFarneback commented Jan 27, 2022

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:

  1. LocalRegistry can use it without any additional dependencies, e.g. for talking to a git service provider's REST API.
  2. No additional configuration or credentials are needed.

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

julia -e 'using Pkg; Pkg.add("LocalRegistry")'
julia --project -e "using LocalRegistry; register(registry = \"$REGISTRY_URL\", create_gitlab_mr = true)"

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.

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2022

Codecov Report

Merging #49 (6ba2200) into master (d847c01) will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/LocalRegistry.jl 96.55% <100.00%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d847c01...6ba2200. Read the comment docs.

Copy link
Contributor

@arnaudh arnaudh left a 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>")
Copy link
Contributor

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.

Suggested change
description = replace(description, "\n" => "<br>")
description = "<ul>"*join(map(x -> "<li>$x</li>", split(description, "\n")))*"</ul>"

Copy link
Owner Author

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)
Copy link
Contributor

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)

?

Copy link
Contributor

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.

Copy link
Owner Author

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.

Copy link
Owner Author

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?

Copy link
Contributor

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

@arnaudh arnaudh mentioned this pull request Feb 17, 2022
@GunnarFarneback GunnarFarneback changed the title WIP: Add option to create a merge request for GitLab. Add option to create a merge request for GitLab. Feb 17, 2022
bors bot added a commit to JuliaRegistries/RegistryCI.jl that referenced this pull request Aug 15, 2022
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>
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.

3 participants