fix: Escape branch names when listing rules - #4534
Conversation
gmlewis
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4534 +/- ##
=======================================
Coverage 98.53% 98.53%
=======================================
Files 195 195
Lines 17851 17851
=======================================
Hits 17590 17590
Misses 261 261 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @nickita-khylkouski!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra
Note that I removed the "!" from "fix!:" since you are not technically breaking the API, although you will be breaking users you already escape the branch name... so we will leave the "BREAKING CHANGE" in the description, but we won't call it out as a breaking API change. I hope that makes sense.
|
Thank you, @alexandear! |
Addresses gmlewis's review: all 23 modified methods across the 5 files touched by this PR now explicitly state, in their doc comment, that the environment name is URL path escaped for the caller -- matching the exact wording and placement already used for the branch parameter in ListRulesForBranch (PR google#4534): // Note: the environment name is URL path escaped for you. See: https://pkg.go.dev/net/url#PathEscape . Placed directly after each method's one-line summary and before its "GitHub API docs:" line, consistent with the branch-name precedent. No logic changed -- doc comments only.
3 participants
hey, ListRulesForBranch puts the branch name straight into the URL, so a valid name like fix/#123-login gets truncated at #. literal % characters can also change the name or make URL parsing fail. this uses url.PathEscape(branch), like the other branch methods and #2948. added regression tests in repos_rules_test.go for ordinary names, slashes, #, and literal %, with and without pagination. BREAKING CHANGE: callers that already escape names as a workaround need to pass the raw branch name instead. this also applies to ListRulesForBranchIter. AI-assisted: ChatGPT helped investigate and draft the fix/tests. the isolated regression checks fail before the fix and pass after; full Go 1.26 repository checks are still pending.