Skip to content
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: Add e2e test for web route prefix #2714

Merged
merged 4 commits into from
Jun 8, 2020

Conversation

onprem
Copy link
Member

@onprem onprem commented Jun 4, 2020

Signed-off-by: Prem Kumar [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Add a new e2e test scenario for testing route prefix. I used chromedp to test failed network requests. Right now this test would fail, because we are using absolute URLs for static assets in our templates, and hence we get 404 Not Found. This is because we are still trying to get assets from /static/* while it should be /{{ routePrefix }}/static/* as we are using a route prefix.

This isn't exactly what we required in #2582 but it serves the same purpose, and is better in some cases. But I think there's still merit in having unit tests for route prefix and similar.

Fixes #2581

Verification

Tested the e2e test by running locally with different scenarios.

Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty good so far, @prmsrswt

test/e2e/query_test.go Show resolved Hide resolved
test/e2e/query_test.go Outdated Show resolved Hide resolved
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me :) however, it seems that there are some conflicts that need to be resolved

@onprem onprem force-pushed the test-route-prefix branch from ab5344a to a689c1b Compare June 4, 2020 19:48
@onprem onprem force-pushed the test-route-prefix branch from a689c1b to 5e8fd47 Compare June 4, 2020 19:56
pkg/ui/ui.go Show resolved Hide resolved
@squat squat merged commit ed71a27 into thanos-io:master Jun 8, 2020
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 awesome work!

squat added a commit to squat/configuration that referenced this pull request Jun 18, 2020
Thanks to thanos-io/thanos#2714, all links in
the frontend are now relative. Unfortunately, that means that using
`--external-prefix=.` turns `static/x` into `.static/x`, which doesn't
exist. We'll fix this here now and then ensure we sanitize this upstream
in Thanos.

Signed-off-by: Lucas Servén Marín <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

query: --web.route-prefix makes UI inaccessible
3 participants