Fix ssh to http conversion for azure devops repos#185
Fix ssh to http conversion for azure devops repos#185mads-oestergaard wants to merge 2 commits intoclearml:masterfrom
Conversation
clearml_agent/helper/repo.py
Outdated
| (?:(?P<user>{regular}*?)@)? | ||
| (?P<host>{regular}*?) | ||
| : | ||
| (?:v3/)? # present in azure ssh urls |
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
| 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 |
|
@mads-oestergaard Is this still of interest? |
3 participants
This PR fixes #184 and also adds a test for dev.azure domains.