-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix not removing tunnel route #1030
Fix not removing tunnel route #1030
Conversation
changelog detected ✅ |
Codecov Report
@@ Coverage Diff @@
## master #1030 +/- ##
==========================================
+ Coverage 49.06% 49.18% +0.12%
==========================================
Files 108 112 +4
Lines 10428 10549 +121
==========================================
+ Hits 5116 5189 +73
- Misses 4200 4232 +32
- Partials 1112 1128 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
there is an alternative here that allows us to wrap this entire line in diff --git utils.go utils.go
index 38d4315..39397bc 100644
--- utils.go
+++ utils.go
@@ -12,7 +12,7 @@ import (
// buildURI assembles the base path and queries.
func buildURI(path string, options interface{}) string {
v, _ := query.Values(options)
- return (&url.URL{Path: path, RawQuery: v.Encode()}).String()
+ return path + (&url.URL{RawQuery: v.Encode()}).String()
}
// loadFixture takes a series of path components and returns the JSON fixture at
diff --git utils_test.go utils_test.go
index 5973d8c..30f7a02 100644
--- utils_test.go
+++ utils_test.go
@@ -27,11 +27,14 @@ func Test_buildURI(t *testing.T) {
"single level path with params": {path: "/bar", params: testExample{C: "d"}, want: "/bar?c=d"},
"single level path with multiple params": {path: "/foo", params: testExample{A: "b", C: "d"}, want: "/foo?a=b&c=d"},
"single level path with nested fields": {path: "/foo", params: testExample{A: "b", C: "d", PaginationOptions: PaginationOptions{PerPage: 10}}, want: "/foo?a=b&c=d&per_page=10"},
+ "escaped URL path": {path: "/foo/10.0.0.0%2F1", params: testExample{A: "b", C: "d"}, want: "/foo/10.0.0.0%2F1?a=b&c=d"},
+ "unescaped URL path": {path: "/foo/10.0.0.0/1", params: testExample{A: "b", C: "d"}, want: "/foo/10.0.0.0/1?a=b&c=d"},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
got := buildURI(tc.path, tc.params)
+
assert.Equal(t, tc.want, got)
})
} |
utils.go
Outdated
@@ -12,7 +12,7 @@ import ( | |||
// buildURI assembles the base path and queries. | |||
func buildURI(path string, options interface{}) string { | |||
v, _ := query.Values(options) | |||
return (&url.URL{Path: path, RawQuery: v.Encode()}).String() | |||
return path + (&url.URL{RawQuery: v.Encode()}).String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my comment must not have been clear. i'm still not sure if we want to do this; it was more of a discussion point. i'm looking into the drawbacks before we swap everything over given most methods use this now and i wouldn't want to make buildURI
less robust for this single (sad) URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Let's do it in a safe way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely changing buildURI may lead to unpredictable bugs in other places
thanks! |
This functionality has been released in v0.47.0. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Description
Fixes #1029
Has your change been tested?
All unit tests are green.
Screenshots (if appropriate):
Types of changes
What sort of change does your code introduce/modify?
Checklist: