-
Notifications
You must be signed in to change notification settings - Fork 11
Shift UI env variables to runtime config #5
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
Conversation
| @@ -0,0 +1,41 @@ | |||
| var xpConfig = { | |||
| "apiConfig": { | |||
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 possible we set default for all (less auth)? im assuming user need to overwrite this file
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.
Yes, user should override this file via configuring uiConfig helm value, and let entrypoint.sh script inject the values during runtime. The helm chart will be added in a follow-up PR.
I've left sensible default values that are commented out for values which are configurable, I think that should suffice? Unless there's certain values you think I should further include.
| set -e | ||
|
|
||
| CMD=() | ||
| XP_UI_CONFIG_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.
should these config be part of https://github.com/gojekfarm/xp/blob/main/CONTRIBUTING.md ?
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.
This will be captured as part of the Helm chart, in deployment.yaml. I'll direct you to this in the subsequent PR.
leonlnj
left a comment
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 see the config are move out of the dockerfile and using entrypoint.sh at init to inject the values. LGTM.
Left some question from the perspective on how a new dev trying to run XP will know how to configure this (maybe I miss this).
Thanks!
|
|
||
| RUN addgroup -S app && adduser -S app -G app | ||
| USER app | ||
| RUN addgroup -S ${XP_USER_GROUP} \ |
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.
any particular reasons why creating user and group is required?
I would thought its for security reasons why the user and group are created to limit which user can execute the binaries in entrypoint.sh, or is this configure there via USER ${XP_USER} ?
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.
By default, containers run as root. A container running as root has full control of the host system. If a service can run without privileges, ideally, we should use a non-root user to restrict privileges. This works in our case, since we're essentially just launching an application service and don’t require root access.
USER does not create a non-root user for use, it simply switches from default ROOT user to the specified non-root user. Hence, we have to create it before using it.
What this PR does / why we need it:
This PR refactors Management Service Dockerfile, to support building the UI separately. This requires injecting runtime configs to the UI rather than relying on
REACT_APP_*environment variables during build time.Which issue(s) this PR fixes:
NONE