-
Notifications
You must be signed in to change notification settings - Fork 336
feat: add fixed basepath /helm-dashboard #606
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
base: main
Are you sure you want to change the base?
Conversation
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 initiative! And well implemented.
I would only ask to not have separate image for that and to make base path configurable. Please see and address my comments.
Let's improve it and merge!
digest: "" | ||
imagePullSecrets: [] | ||
# Flag for using an image with fixed base path /helm-dashboard | ||
basePath: false |
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.
Can we make this parameter string instead of boolean? This way user would be able to set any base path he wants, not only helm-dashboard
. More flexible.
build-args: VER=${{ needs.pre_release.outputs.release_tag }} | ||
platforms: linux/amd64,linux/arm64 | ||
|
||
- name: Build and push with base path |
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.
Is it absolutely necessary to have a separate image for that?
From the first look it seems that just setting env variable should trigger the behavior. We have the ways to communicate configured base path from backend to frontend (via StatusInfo
struct).
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.
I'm +1 on having only one image and have configurable base path. I could not do it that way becasue VITE_BASE_PATH
is only accesible at build time AFAIK. Once the image is built, we can only customize the base path for the backend via env var or as a flag as you suggest.
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.
After reading this doc part: https://vite.dev/guide/build.html#relative-base
I have a feeling that we could just use
export default defineConfig({
base: './'
})
And it would do the trick universally. Did you try investigating that direction?
done := s.startBackgroundServer(api, ctx) | ||
|
||
return "http://" + s.Address, done, nil | ||
basePath := strings.TrimSpace(os.Getenv("HD_BASE_PATH")) |
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.
Instead of calculating base path here, make it a field in Server
struct and pass the value from main.go. Because this option looks exactly like server start config option. This way we have single place that calculates the option value (main.go) and then others use it from field/variable.
Value of this option would then also go into NewRouter
call here.
|
||
configureStatic(api) | ||
configureRoutes(abortWeb, data, api) | ||
// Determine base path prefix for mounting the app under a subpath |
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.
see my comment in server.go - the code would be simpler if the base path value is just passed to NewRouter()
{{- if .Values.image.basePath }} | ||
- name: HD_BASE_PATH | ||
value: /helm-dashboard | ||
{{end}} |
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.
Assuming base path is string, setting VITE_BASE_PATH
here would be better chart design, not requiring separate image
Changes Proposed
Allow app to serve under fixed base path
/helm-dashboard
Check List