-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
feat(args): allow GitLab groups with --gitlab-repo
#807
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
==========================================
+ Coverage 39.64% 39.79% +0.15%
==========================================
Files 20 20
Lines 1607 1611 +4
==========================================
+ Hits 637 641 +4
Misses 970 970
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for the PR! We should definitely include this fix in the upcoming release.
Yeah, the current state of this PR is breaking since it splits on the last occurrence. We need to somehow support both cases. |
let parts = value.split('/').rev().collect::<Vec<&str>>(); | ||
if let (Some(owner), Some(repo)) = (parts.get(1), parts.first()) { | ||
Ok(RemoteValue(Remote::new(*owner, *repo))) | ||
let parts = value.rsplit_once('/'); |
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 idea is to parse it as URL then split it (just to be safe):
let parts = value.rsplit_once('/'); | |
let mut value = inner.parse_ref(cmd, arg, value)?; | |
if let Ok(url) = Url::parse(&value) { | |
value = url.path().trim_start_matches('/').to_string(); | |
} | |
let parts = value.rsplit_once('/'); |
(you need to add url
crate to the workspace dependencies)
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'm planning a release soon so I will merge this as-is, thank you!
--gitlab-repo
Congrats on merging your first pull request! ⛰️ |
Sorry, did read it just now. Thanks a lot for adjusting the code and merging it so quickly. And of course for building such an amazing tool! ❤️ |
Description
The cli argument
--gitlab-repo
is split at/
and the last two parts are used asowner
andrepo
. This PR changes this behavior, splits at the right most/
and takes the first part asowner
and the second part asrepo
.gitlab/group/a/repo_name
a
repo_name
gitlab/group/a/repo_name
gitlab/group/a
repo_name
Motivation and Context
GitLab allows projects to be organized in groups. Groups can also be part of groups. Because of that projects are not identified by
owner
andrepo
, but bygroup/path
andrepo
.The same behavior can be achieved by using the following inside the config:
But since I would like to use the same config in multiple projects, I rather provide the
remote
information via an environment variable.How Has This Been Tested?
I tested this with GitLab projects that are organized in groups.
There was a testcase that ignored the
https://github.com
domain if it was included. Since I never saw this inside the documentation I simply removed it, but I'm happy to adjust this PR in a way that this test would still pass, if needed.Screenshots / Logs (if applicable)
Types of Changes
Checklist: