Escape package names to support names which include a slash#3002
Escape package names to support names which include a slash#3002gmlewis merged 6 commits intogoogle:masterfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3002 +/- ##
=======================================
Coverage 97.91% 97.91%
=======================================
Files 150 150
Lines 12980 12980
=======================================
Hits 12710 12710
Misses 192 192
Partials 78 78 ☔ View full report in Codecov by Sentry. |
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @bn4t !
For each of the modified methods, could you please add a godoc comment (since this is a change of behavior) and also a unit test that demonstrates the new path escaping?
For example:
// Note that packageName is escaped for the URL path so that you don't need to.|
@gmlewis Would it be fine if I just changed the existing tests to use a package name with a slash or do you want me to write one/multiple new tests for this? |
1f84f2c to
8cc816b
Compare
gmlewis
left a comment
There was a problem hiding this comment.
Thanks, @bn4t .
Please don't force-push as it makes it challenging for reviewers to see what changed since the last review. Also, we always squash-and-merge in this repo, so it doesn't matter how many pushes are in a PR - they will all end up as one commit when we merge.
I think it is totally fine to modify the existing tests. Thanks. |
Changed fmt.Fprint to io.WriteString because go vet complained about possible Printf formatting directive.
|
Please run step 4 of our CONTRIBUTING.md guide and push the results to this PR. That should fix the linter errors. |
|
@gmlewis Okay, pushed it. Thanks for handholding me through this, this really shouldn't have taken so long. My fault for not checking the contributing guidelines in detail. |
No problem, we do have a lot of automation in this repo that is not immediately obvious. |
|
Thank you, @valbeat ! |
Package names aren't escaped before being added to the url.
E.g.
orgs/foo/packages/container/foo/barinstead oforgs/foo/packages/container/foo%2FbarThis causes 404 errors for such packages.