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

Allow overriding base path for UI/API routes; rm --query.prefix #748

Merged
merged 7 commits into from Mar 23, 2018

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Mar 20, 2018

Resolves #745 Provide a means to define a path-prefix for the UI
Resolves #746 Remove --query.prefix option

@ghost ghost assigned yurishkuro Mar 20, 2018
@ghost ghost added the review label Mar 20, 2018
@yurishkuro yurishkuro changed the title Allow overriding base path for UI/API routes Allow overriding base path for UI/API routes; rm --query.prefix Mar 20, 2018
@@ -1,6 +1,7 @@
<!DOCTYPE html>
<html lang="en">
<meta charset="UTF-8">
<base href="/" data-inject-target="BASE_URL"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

@tiffon this models the compiled index.html from UI assets.

Q: is it possible that babel/webpack would reorder the href and data-inject-target attributes? Because if it does, the search&replace in the query service won't work

Copy link
Member

Choose a reason for hiding this comment

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

@yurishkuro It should not change the order of the attributes.

@coveralls
Copy link

coveralls commented Mar 20, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7a019df on fix-745-support-base-path into 37e6430 on master.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM, but would be awesome to have the user notified when the HTML on disk is not what is being served to clients. Also, it would perhaps be worth adding some piece of documentation about this (when to use/why/how)

options.BasePath = "/"
}
if options.BasePath != "/" {
if !strings.HasPrefix(options.BasePath, "/") || strings.HasSuffix(options.BasePath, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a log statement notifying that what people see in the HTML is not what will be served to clients?

Copy link
Member Author

Choose a reason for hiding this comment

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

could you elaborate why you think this would be useful? We're modifying the internal file of the app, not something that users provide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mainly because it's deviating from what is "expected": if I had a problem with the static resources (like the original problem this PR is solving) and were to check the HTML, I would wonder why it's not the same as what my browser is seeing. A log entry at debug level would suffice, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, still not convinced why someone needs to know what happens internally, e.g. we also override the config, and in both cases we only do that when requested via cli arguments.

Let's do it separately, because the constructor doesn't take a logger and the unit tests explicitly verify that nothing is logged, so it's not as trivial a change as it sounds for a feature that seems rather marginal.

@yurishkuro
Copy link
Member Author

Docs would be updated in a separate PR in the docs repo.

Yuri Shkuro added 7 commits March 23, 2018 14:59
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit 83d06da into master Mar 23, 2018
@ghost ghost removed the review label Mar 23, 2018
@yurishkuro yurishkuro deleted the fix-745-support-base-path branch March 23, 2018 19:32
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.

Remove --query.prefix option Provide a means to define a path-prefix for the UI
5 participants