-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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(admin) fix regression of trailing slash in targets uri #2884
Conversation
With the trailing slash the `.../targets/active` endpoint would return a "method not allowed" error fixes #2840
1bdcac7
to
7644acd
Compare
@@ -118,7 +118,7 @@ return { | |||
end, | |||
}, | |||
|
|||
["/upstreams/:upstream_name_or_id/targets/active/"] = { |
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.
I believe (because I tested this yesterday) that removing this makes the endpoint works with both a trailing slash and without. We should also write a new regression test that ensures it is the case (after all, this is a regression we don't want to see happening again in the future).
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.
yep, good idea
}) | ||
assert.response(res).has.status(200) | ||
local json = assert.response(res).has.jsonbody() | ||
for _, append in ipairs({ "", "/" }) do |
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.
Not a blocker but fwiw, I don't think this is a healthy way of writing tests. If one behavior fails (with or without trailing slash), the developer sees a test named "only shows active targets" fail, which isn't what the test was actually asserting. On top of that, the developer has no easy way of knowing which of the two tests (the one with or without the trailing slash) failed, which is confusing and/or frustrating.
Tests must be atomic and explicit, always. "Meta-programming" testing (generating tests based on loops) should be avoided when it breaks the atomicity of the test.
With the trailing slash the
.../targets/active
endpoint would return a "method not allowed" errorfixes #2840