deck: add base path plumbing and helper APIs #576
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tsj-30 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @tsj-30. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
CC: @petr-muller , @matthyx |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/retest |
|
Hi @petr-muller , |
|
cc: @loispostula |
|
@tsj-30 I'm not sure how to test this, I'll try to get a local build working and check |
|
Hi @loispostula, |
|
/cc I have an in-flight review with some feedback, will post hopefully tomorrow |
k8s-ci-robot
added
the
do-not-merge/hold
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
My main concern is how well this was tested on the current default case (path is "/")? If the new subpath functionality is not 100% then I don't mind because we can fix it as we go, but the one thing we need to prevent is breaking existing deployments.
Some comments inline
/hold cancel
|
|
||
| func gatherOptions(fs *flag.FlagSet, args ...string) options { | ||
| var o options | ||
| o.basePath = "/" |
There was a problem hiding this comment.
nit: this is redundant with setting a default on L192
| } | ||
|
|
||
| func (o *options) Validate() error { | ||
| o.basePath = normalizeBasePath(o.basePath) |
There was a problem hiding this comment.
seems redundant with doing the same in gatherOptions
| // withBasePath ensures that only requests that begin with the configured base path are handled | ||
| // and that downstream handlers only see paths relative to that base path. | ||
| func withBasePath(basePath string, handler http.Handler) http.Handler { | ||
| basePath = normalizeBasePath(basePath) |
There was a problem hiding this comment.
another normalizeBasePath call
| "deckBasePath": func() string { return o.basePath }, | ||
| "deckPath": func(p string) string { return o.absolutePath(p) }, |
There was a problem hiding this comment.
How are these consumed?
| // absolutePath returns the absolute URL path for a given relative path by prepending the | ||
| // configured base path. The input is expected to be either "/" or begin with a slash. | ||
| func (o options) absolutePath(rel string) string { | ||
| if rel == "" { | ||
| return "" | ||
| } | ||
|
|
||
| pathPart := rel | ||
| suffix := "" | ||
| if idx := strings.IndexAny(rel, "?#"); idx >= 0 { | ||
| pathPart = rel[:idx] | ||
| suffix = rel[idx:] | ||
| } | ||
| if pathPart == "" { | ||
| pathPart = "/" | ||
| } | ||
| if !strings.HasPrefix(pathPart, "/") { | ||
| pathPart = "/" + pathPart | ||
| } | ||
| if o.basePath == "/" { | ||
| return pathPart + suffix | ||
| } | ||
| if pathPart == "/" { | ||
| return o.basePath + suffix | ||
| } | ||
| return o.basePath + pathPart + suffix | ||
| } |
There was a problem hiding this comment.
It's not easy to reason about all path manipulation possibilities, I'd like to have unit tests (also for stripBasePath and normalizeBasePath both to make sure they are correct and to serve as examples of what gets transformed to what.
|
|
||
| // absolutePath returns the absolute URL path for a given relative path by prepending the | ||
| // configured base path. The input is expected to be either "/" or begin with a slash. | ||
| func (o options) absolutePath(rel string) string { |
There was a problem hiding this comment.
This technically only needs to basePath from options, could the field be a custom string type and the methods would be defined on that? It would make the interface more narrow.
k8s-ci-robot
removed
the
do-not-merge/hold
6 participants
This change teaches Deck about a configurable base path so it can be served from a sub‑URL without breaking static assets, APIs, or Spyglass. It introduces the --base-path flag, wraps the HTTP mux so requests are stripped/prefixed appropriately, extends template helpers to emit base-aware links, and adds deckURL utilities for the front-end.
This fixes #376.