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
Set the value of gitreview.username from the gerrit push URL #11585
Set the value of gitreview.username from the gerrit push URL #11585
Conversation
1f8afb0
to
6d099da
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.
Can you please also add tests to verify this actually behaves as intended?
weblate/vcs/git.py
Outdated
Gets the gerrit username from push URL and | ||
sets it as the value of gitreview.username. | ||
""" | ||
gerrit_user = push_url.split("@")[0].split("//")[1] |
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.
This needs to be more robust to handle all possible URL schemes. Your code will crash on SSH push URL like git@github.com:WeblateOrg/weblate.git
.
See parse_repo_url
method above in this file for an example that work well.
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.
Actually it even fails within the testsuite: https://github.com/WeblateOrg/weblate/actions/runs/8979415860/job/24661366078?pr=11585#step:7:4068
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 will improve this to handle possible URL schemes. It was initially assumed that URLs coming from Gerrit will look like this:
ssh://[user]@[host]:[port]/[repo].git
Actually, it has different URL schemes.
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.
AFAIK this can be any URL Git supports.
1995240
to
501a1bc
Compare
weblate/vcs/git.py
Outdated
if push_url.startswith("ssh://"): | ||
gerrit_user = push_url.split("@")[0].split("//")[-1] | ||
elif push_url.startswith("git@") or push_url.startswith("https://"): | ||
_, _, gerrit_user, _ = self.parse_repo_url(push_url) |
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 don't think that owner as returned by parse_repo_url is what you want here - it is owner of the repo at GitHub (for https://github.com/WeblateOrg/weblate.git
it would be WeblateOrg
).
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.
Hi @nijel, I updated how it was implemented. I also added test with different URLs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11585 +/- ##
==========================================
- Coverage 90.82% 90.58% -0.24%
==========================================
Files 554 570 +16
Lines 57306 58643 +1337
Branches 9122 9383 +261
==========================================
+ Hits 52046 53122 +1076
- Misses 3640 3836 +196
- Partials 1620 1685 +65
|
f6f1df0
to
e14736a
Compare
weblate/vcs/git.py
Outdated
gerrit_user = self.get_username_from_url(push_url) | ||
self.config_update(("gitreview", "username", gerrit_user)) | ||
super().configure_remote(pull_url, push_url, branch, fast) | ||
|
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.
Reading this code again, I've just realized that better place for this is get_remote_configure
because it will parse/update the configuration file at once and not multiple times as we do it now.
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 agree with this @nijel . I removed the override implementation of configure_remote
inside GitWithGerritRepository
and transferred the setting of gitrefview.username
value to the get_remote_configure
.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
43c08fd
to
a34a987
Compare
for more information, see https://pre-commit.ci
Merged, thanks for your contribution! |
Proposed changes
This will automatically get the username from the push URL when using Gerrit as VCS
This solves issue #10596
Checklist
Other information