-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
UI: Fix infinite redirection loop on / #4208
Conversation
Since this bug was introduced during v0.20.0, does it makes sense to release 0.20.2 with this fix included? cc @kakkoyun |
Yes, I'd say let's release a minor version with the patch. |
I think I need to create a PR to the |
Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>
Finally managed to change the base correctly 😵 |
The e2e tests are failing because of the busybox SHA pinning issue (which is fixed on main). |
Right, however to release the version we need to pull those in too. |
Are there any tests we could add? I remember how we've had lots of different fixes for the prefix stuff and the issue wasn't completely solved until tests were added that actually programmatically check whether everything works correctly 😓 |
As @GiedriusS suggested it'd be great to have tests. About #4171 and #4202, do we need them for the end artifact or just for CI to pass? I don't have enough context on this. |
I am sorry for the delay on this. I don't have enough context either, but I think we need them for both CI and the end artifact as we won't be able to create docker images to push. I am not even sure if both of them are required or if we can only use one of them. But since the next release is very soon (first RC in 6 days I think), can we just merge this to main and go ahead. Update: On a closer look, seems like #4171 was effectively a no-op as the arg was overridden in Makefile, and that was updated in #4202. For now, I'll cherry-pick both of them here and see if we can make the CI happy. |
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@GiedriusS Yes, we do have tests for that, but I am not sure how can we add tests for these type of scenarios. There are a lot of places where things can go wrong 😅 The E2E tests for routePrefix etc would have caught this but, but they access the |
Depandabot CI is saying that the config file is wrong and it doesn't have anything like |
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.
LGTM 🚀
* UI: Fix infinite redirection loop on / Signed-off-by: Prem Saraswat <prmsrswt@gmail.com> * Updated busybox sha; Added dependabot. (thanos-io#4202) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed dependabot yml. (thanos-io#4205) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
* UI: Fix infinite redirection loop on / (#4208) * UI: Fix infinite redirection loop on / Signed-off-by: Prem Saraswat <prmsrswt@gmail.com> * Updated busybox sha; Added dependabot. (#4202) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed dependabot yml. (#4205) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> * Cut release v0.20.2 (#4262) Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> Co-authored-by: Prem Saraswat <prmsrswt@gmail.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Changes
/
instead of figuring out the path to redirect to. This fixes Block Viewer: --web.route-prefix flag inconsistent/unexpected redirects #4128/
prefix to other redirects. Without this the location is interpreted as relative and we end up doing weird redirections like/prefix/new/prefix/...
.Verification
Tested Querier, Ruler, and Block Viewer UI locally with and without external prefixes