Conversation
|
@gmlewis could you please approve actions test to see if all works correctly? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3459 +/- ##
=======================================
Coverage 91.03% 91.03%
=======================================
Files 179 179
Lines 15535 15535
=======================================
Hits 14142 14142
Misses 1221 1221
Partials 172 172 ☔ View full report in Codecov by Sentry. |
Yes, however we don't want to remove problematic (but valid) branch names from the unit tests (as you have done in this PR)... |
I am not sure if I get what you're saying, I haven't really removed any branch names, instead in the mux urls(the one's to be called) by api, I have just Path escaped them, github also does this, for eg, This would go like this:
Also see, tests in net/http. IMO, one thing that could be done better here is,to create a util function of sort that would take the branch name and path escape them before adding it to mux route. |
Ah! I see now what you are saying, @amanv8060. I apologize for the confusion I caused. Yes, a utility function would help tremendously, I would think, and make it clearer what is happening. Would you mind doing that as part of this PR? |
|
I would prefer to do that in seperate pr so that review becomes easier. |
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @amanv8060 !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@stevehipwell - might you have time for a code review? Thanks!
|
Thank you, @stevehipwell and @alexandear ! |
Use correct url escaping to fix the errors caused by mux changes.
Fixes: #3409