Fix ssh to http conversion for azure devops repos #185
Conversation
clearml_agent/helper/repo.py
Outdated
| @@ -225,6 +225,7 @@ def remote_branch_name(branch): | |||
| (?:(?P<user>{regular}*?)@)? | |||
| (?P<host>{regular}*?) | |||
| : | |||
| (?:v3/)? # present in azure ssh urls | |||
There was a problem hiding this comment.
@mads-oestergaard how is this actually used?
There was a problem hiding this comment.
It keeps :v3/ out of the path group.
There was a problem hiding this comment.
It's used down in line 254
clearml_agent/helper/repo.py
Outdated
| @@ -253,6 +254,16 @@ def get_username(user_, password=None): | |||
| match = cls.SSH_URL_GIT_SYNTAX.match(url) | |||
| if match: | |||
| user, host, path = match.groups() | |||
|
|
|||
| # handle special azure cases | |||
| if "ssh" and "azure" in host: | |||
There was a problem hiding this comment.
This seems pretty specific and I'm not sure we can count on it not appearing in other URLs - I think we need a better way to detect that
There was a problem hiding this comment.
For sure. I'm just not aware of other URLs that has this pattern.
Generally I guess that you wouldn't expect [.?]ssh[.?] to show up in the host path in any case and then we only need to insert the /_git/ thing for azure URLS?
There was a problem hiding this comment.
We could also add a match in the SSH_URL_GIT_SYNTAX regex that filters out ssh. from the path group and then only handle the special _git component here. WDYT @jkhenning?
There was a problem hiding this comment.
To make it more general, I suppose one could filter for v followed by a single digit, instead of v3 directly, to mitigate v4 and v5 etc. It does feel over-engineered though.
There was a problem hiding this comment.
@mads-oestergaard I think we should just go with an exact string representing the Azure URL (this can be in the default configuration so people might override it if things change)
|
@jkhenning any thoughts on this? |
|
Hi @mads-oestergaard, as I mentioned in my last comment, I'd like to change this to check on the actual Azure URL (as it's a fixed URL anyway, and relying on opportunistic parsing is something I'd like to keep in the code) |
|
Hi @jkhenning thanks for answering. I got caught up with other things, but will give this a look again sometime next week |
This PR fixes #184 and also adds a test for dev.azure domains.