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

server: templates: use relative URLs to refer to assets #1554

Merged

Conversation

yanniszark
Copy link
Contributor

@yanniszark yanniszark commented Sep 24, 2019

Fixes #1553

This PR makes sure every template refers to its local assets using a URL relative to the request's URL.

For example:
server listens at localhost/dex so serverPath is dex
reqPath is /dex/auth
assetPath is static/main.css
relativeURL("/dex", "/dex/auth", "static/main.css") = "../static/main.css"

I had to propagate the request's path from each handler down to the templates' data.
I chose to pass the whole http request as a parameter to the template render functions, as it may contain other useful information in the future (eg values from context).
I can change it to only pass down the request's path if it's preferable.

Signed-off-by: Yannis Zarkadas [email protected]

@yanniszark yanniszark force-pushed the feature-web-templates-use-relative-urls branch 5 times, most recently from 97b0a2d to e67635c Compare September 27, 2019 18:27
@yanniszark
Copy link
Contributor Author

Hi @srenatus!
This PR is ready for a review whenever you have the time.
I wrote some unit tests for the new function that calculates the relative URL and I have also tested it in a Kubernetes cluster.

@yanniszark yanniszark force-pushed the feature-web-templates-use-relative-urls branch 2 times, most recently from c450cf5 to 4c0030d Compare October 2, 2019 13:54
@yanniszark yanniszark force-pushed the feature-web-templates-use-relative-urls branch from 4c0030d to 69d13b7 Compare October 2, 2019 14:08
@yanniszark
Copy link
Contributor Author

I just noticed that @srenatus is on parental leave. Sorry for the disturbance.

/cc @JoelSpeed @bonifaido @rithujohn191
Could you please take a look if possible?
This is something that we need for our use-case in integrating Dex with the Kubeflow project.

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Seems sensible to me, one quick question, I don't mind this style just curious as to why you went for it 🙂

//relativeURL("/dex", "/dex/auth", "static/main.css") = "../static/main.css"
func relativeURL(serverPath, reqPath, assetPath string) string {

splitPath := func(p string) []string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to define this and stripCommonParts as a local variable rather than an unexported method in the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the review @JoelSpeed 😄
I defined them as closures inside the relativeURL function because their scope was that specific function.
That is, they are not used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough 👍

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.

web templates: use relative URLs for static assets
2 participants