-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Single source of configuration I/II #4882
Conversation
bf2078c
to
4fdb305
Compare
ef42fc0
to
74d3a80
Compare
@Goldselleruk: changing LGTM is restricted to collaborators In response to this:
Instructions 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/test-infra repository. |
/werft run 👍 started the job as gitpod-build-gpl-4650-server-config.8 |
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.
Code changes look good. Thanks @geropl!
Let's resolve the merge conflicts and perhaps wait with merging for todays deployment in order to verify on staging one more time.
c1c4323
to
c3d7b2a
Compare
To be able to easily map Env+EnvEE into config there was some minor refactoring/cleanup necessary. This has be done in a way to be as straigth forward as possible to minimize the risk of breaking things, while making it possible to easily write an alternative Config parser.
c3d7b2a
to
25a5fd6
Compare
@AlexTugarev I rebased, merged Env/EnvEE -> Config and squashed commits: ready to merge! |
@@ -80,6 +82,7 @@ export class Server<C extends GitpodClient, S extends GitpodServer> { | |||
|
|||
public async init(app: express.Application) { | |||
log.info('Initializing'); | |||
log.info('config', { config: JSON.stringify(this.config, undefined, 2) }); |
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.
👍🏻
/hold /lgtm |
LGTM label has been added. Git tree hash: 7f2752ee0e556fad6c7ea68ed6e637e559627209
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexTugarev, geropl, Goldselleruk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@geropl feel free to release the break ;-) |
Attempts to fix #4650. This is the 1st in a series of 2 PRs.
It turns out the helm chart and parsing interaction in
Env
is rather complicated. To make sure we do not miss any variance we take a 2 step approach:Config
shape which contains everything fromEnv
(minus some basic refactoring and cleanup). This is not used yet to make sure we don't break anything, but instead just printed out on startup for reference.Config
, ideally reducing it to a{{ toJson | b64enc }}
on helm followed by aJSON.parse(...)
+ validation inserver
. To make sure we don't break stuff we can compare the result to the print-outs fromstaging
anddevstaging
For this PR, feedback is very welcome on:
Config
Env
can be removed.The rest is straight forward extract/removal.