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

Set the value of gitreview.username from the gerrit push URL #11585

Merged

Conversation

harriebird
Copy link
Collaborator

Proposed changes

This will automatically get the username from the push URL when using Gerrit as VCS
This solves issue #10596

Checklist

  • Lint and unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added documentation to describe my feature.
  • I have squashed my commits into logic units.
  • I have described the changes in the commit messages.

Other information

Copy link
Member

@nijel nijel left a 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?

Gets the gerrit username from push URL and
sets it as the value of gitreview.username.
"""
gerrit_user = push_url.split("@")[0].split("//")[1]
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@harriebird harriebird May 7, 2024

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.

Copy link
Member

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.

@harriebird harriebird force-pushed the fix/config-gitreview-username-from-ssh-url branch from 1995240 to 501a1bc Compare May 7, 2024 17:12
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)
Copy link
Member

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).

Copy link
Collaborator Author

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.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.58%. Comparing base (6aee586) to head (e91d308).
Report is 1720 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
weblate/vcs/tests/test_vcs.py 99.21% <100.00%> (+0.01%) ⬆️
weblate/vcs/git.py 85.70% <83.33%> (-0.44%) ⬇️

... and 244 files with indirect coverage changes

@harriebird harriebird force-pushed the fix/config-gitreview-username-from-ssh-url branch 2 times, most recently from f6f1df0 to e14736a Compare May 9, 2024 14:07
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)

Copy link
Member

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.

Copy link
Collaborator Author

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.

@harriebird harriebird force-pushed the fix/config-gitreview-username-from-ssh-url branch from 43c08fd to a34a987 Compare May 10, 2024 09:50
@nijel nijel added this to the 5.6 milestone May 13, 2024
@nijel nijel merged commit 762ba8a into WeblateOrg:main May 13, 2024
31 checks passed
@nijel
Copy link
Member

nijel commented May 13, 2024

Merged, thanks for your contribution!

@harriebird harriebird deleted the fix/config-gitreview-username-from-ssh-url branch May 13, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants