-
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
query: Fix external prefix and add an e2e test for it #2800
Conversation
Signed-off-by: Prem Kumar <[email protected]>
a2f811e
to
280299e
Compare
Signed-off-by: Prem Kumar <[email protected]>
280299e
to
f5bff42
Compare
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
Now that we are more in line with what Prometheus does, the following guides also works for Thanos.
The only difference is that, instead of using the Prometheus flag |
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.
Awesome work! Just a few comments on how to improve this 🎉
Signed-off-by: Prem Kumar <[email protected]>
87f9ea1
to
eb4dff8
Compare
Seems like Netlify is having some problems. |
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.
The code looks good! I think we should another iteration on the documentation and then we can merge this :P Mostly grammar nits + renamed Thanos querier to Thanos Query to be consistent.
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
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. Even tested with a reverse proxy with the example configuration in the documentation and it works with these changes whereas with the master
version you get redirected to http://localhost/thanos/thanos/graph
with identical configuration.
I'll give @squat and others time to review these changes if they want to before merging this. Awesome stuff!
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.
Looks great overall, Prem! Great work and thanks for the docs! Just some small nits
Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Prem Kumar <[email protected]>
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.
Great 🎉
Oops, fixing up the CHANGELOG in #2833. |
Changes
--web.external-prefix
using a simple reverse proxy--web.external-prefix
and--web.route-prefix
in a single scenario, i.e. when both are set simultaneously.Changes in
externalPrefix
androutePrefix
handling--web.route-prefix
defaults to--web.external-prefix
. This behavior allows a user to usethanos
with and without a reverse proxy if--web.external-prefix
is set, and it is in line with what Prometheus does.--web.external-prefix
Turns out,--web.external-prefix
is still broken, and as expected, these tests would fail unless we fix it.It should work now.
This PR changes how
--web.external-prefix
works, so it can make the UI inaccessible to some users who were relying on some workarounds or hacks before (like setting--web.external-prefix="."
).Verification
Ran
GOTEST_OPTS="-run TestQuery" make test-e2e
locally to test all e2e tests related to Query component.