Fix issue-743 by sanitizing the URL #809
Conversation
Avish34
force-pushed
the
fix-issue-743
branch
5 times, most recently
from
2e4a9ce to
5118c3b
Compare
|
@tadasant @domdomegg Please review!! |
|
@claude please review this PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
tadasant
left a comment
tadasant
left a comment
There was a problem hiding this comment.
I'm not sure introducing a utils pattern is necessary here. Can we just add it to the current flow in detectRepoURL?
cmd/publisher/publisher.exe
Outdated
There was a problem hiding this comment.
@tadasant Pushed the changes, please review.
sure!! |
Avish34
force-pushed
the
fix-issue-743
branch
2 times, most recently
from
543d864 to
cc55e8d
Compare
cmd/publisher/commands/init.go
Outdated
| } | ||
|
|
||
| func detectRepoURL() string { | ||
| func detectRepoURL(packageType string) string { |
There was a problem hiding this comment.
Is it necessary to pipe around packageType here? Couldn't we just "strip git+ from the prefix if it exists" regardless of the packageType and keep this change fairly minimal? Or is there some case you're imagining where we'd want git+ as a prefix?
There was a problem hiding this comment.
No, I was just thinking that if someday going forward a case arises for a different package manager where we need to do some changes to repoURL generated by them. In that case, the changes will be minimal a that time. I'm happy to remove the packageType params if you think it won't be much helpful right now. Happy to address the comments, let me know your thoughts.
There was a problem hiding this comment.
I'd rather we do the simpler change now and do the bigger change later if the need arises (I'm not sure it definitely will). Thank you!
There was a problem hiding this comment.
unrelated failure in sanity, good to merge.
Avish34
force-pushed
the
fix-issue-743
branch
from
cc55e8d to
e5b7b50
Compare
Avish34
force-pushed
the
fix-issue-743
branch
from
e5b7b50 to
28c58dc
Compare
2 participants
This PR aims to fix #743 (comment)
Motivation and Context
This is needed to ensure we are sanitizing the URL before creating server.json to be inline with the validations.
How Has This Been Tested?
Tested via UTs and also tested CLI locally to ensure the expected behavior.
Breaking Changes
Types of changes
Checklist
Additional context