Skip to content

Escape package names to support names which include a slash - #3002

Merged
gmlewis merged 6 commits into
google:masterfrom
bn4t:fix-escape-package-names
Dec 16, 2023
Merged

Escape package names to support names which include a slash#3002
gmlewis merged 6 commits into
google:masterfrom
bn4t:fix-escape-package-names

Conversation

@bn4t

@bn4tbn4t commented Nov 24, 2023

Copy link
Copy Markdown
Contributor

Package names aren't escaped before being added to the url.
E.g. orgs/foo/packages/container/foo/bar instead of orgs/foo/packages/container/foo%2Fbar

This causes 404 errors for such packages.

@codecov

codecovBot commented Nov 24, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (50d4db2) 97.91% compared to head (5a5a215) 97.91%.
Report is 3 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@bn4t

bn4t commented Dec 1, 2023

Copy link
Copy Markdown
ContributorAuthor

@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?

@bn4t
bn4tforce-pushed the fix-escape-package-names branch from 1f84f2c to 8cc816bCompareDecember 1, 2023 09:51

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment threadgithub/orgs_packages.go Outdated
@gmlewis

Copy link
Copy Markdown
Collaborator

@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?

I think it is totally fine to modify the existing tests. Thanks.

Benjamin Nater added 2 commits December 4, 2023 10:58
Changed fmt.Fprint to io.WriteString because go vet complained about possible Printf formatting directive.
@bn4t
bn4t requested a review from gmlewisDecember 4, 2023 11:03
@gmlewis

Copy link
Copy Markdown
Collaborator

Please run step 4 of our CONTRIBUTING.md guide and push the results to this PR. That should fix the linter errors.
Thanks.

@bn4t

bn4t commented Dec 4, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@gmlewis

Copy link
Copy Markdown
Collaborator

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

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @bn4t !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Dec 4, 2023

@valbeatvalbeat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Dec 16, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @valbeat !
Merging.

@gmlewis
gmlewis merged commit ee55955 into google:masterDec 16, 2023
gmlewis pushed a commit to WillAbides/go-github that referenced this pull request Dec 18, 2023
gmlewis pushed a commit to o-sama/go-github that referenced this pull request Dec 19, 2023
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@bn4t@gmlewis@valbeat