Skip to content

Conversation

@trakos
Copy link
Contributor

@trakos trakos commented Jun 13, 2021

Hey,

first of all, thanks for the great package. I found a minor bug when using it with gitlab repositories.

Gitlab allows creating subgroups, for instance here is one of the gitlab repostiories: https://gitlab.com/gitlab-org/ci-cd/shared-runners/images/gcp/windows-containers . The source git path for that repo has many / signs:

git@gitlab.com:gitlab-org/ci-cd/shared-runners/images/gcp/windows-containers.git

The regex that is used to substitute package url does not allow for that, it only allows single / sign in path. As a result, it generated a line like that for me:

| istruct/component-developer       | Downgraded | 0.5.2 | dev-feature/tools a463df1 | [Compare](git@git.mydomain.example.com:istruct/component/developer/compare/0.5.2...a463df1)    |

As you can see, it left the git@ part in the comparison link, because preg_replace pattern didn't match anything.

I propose a simple fix that allows / in repository namespaces.

Additionally, comparison links across forks like:

https://gitlab.acme.org/IonBazan/package/compare/3.12.0...acme:3.12.1

don't work on gitlab for me, they have slightly different syntax:

https://gitlab.acme.org/IonBazan/package/compare/3.12.0...3.12.1?from_project_id=27069907

Unfortunately, we cannot generate it without knowing internal numerical project id, so I propose to simply return null in gitlab links generator when project namespaces don't match.

@trakos trakos force-pushed the fix-gitlab-subgroup branch from efd30d3 to bd444dd Compare June 13, 2021 17:29
@codecov
Copy link

codecov bot commented Jun 13, 2021

Codecov Report

Merging #3 (b2ab777) into master (ae1fca2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master        #3   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       133       133           
===========================================
  Files             16        16           
  Lines            364       365    +1     
===========================================
+ Hits             364       365    +1     
Impacted Files Coverage Δ
src/Url/GitGenerator.php 100.00% <100.00%> (ø)
src/Url/GitlabGenerator.php 100.00% <100.00%> (ø)

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 ae1fca2...b2ab777. Read the comment docs.

Copy link
Owner

@IonBazan IonBazan 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 your contribution. Just one small change requested.
It's a pity we are not able to generate a proper compare URLs for Gitlab forks.

There is also one new escaped mutant in mutation tests but I will look into that after we merge this one.

@IonBazan IonBazan merged commit be4aabc into IonBazan:master Jun 14, 2021
@IonBazan
Copy link
Owner

Thanks @trakos! ❤️

@trakos
Copy link
Contributor Author

trakos commented Jun 14, 2021

I appreciate the quick merge @IonBazan ! 🙇 Your change definitely makes sense, it's better to give link to the target release.

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.

2 participants