-
Notifications
You must be signed in to change notification settings - Fork 503
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
Adding a config.toml value to the helm chart #1420
Conversation
One thing is missing is that we need chart version bump in |
@@ -82,6 +82,16 @@ gitconfig: | |||
# Key in the kubernetes secret that contains git config data. | |||
secretKey: gitconfig | |||
|
|||
configToml: | |||
enabled: 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.
Do you prefer enabled
-switch approach?
Why not to just always use this config, without any conditions? I guess the default values should be fine, so we can leave it empty as you did. If not, we can provide some useful settings, maybe from https://github.com/gomods/athens/blob/master/config.dev.toml but still always mouning the config to the container.
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.
@hypnoglow I set enabled: false
to keep the Kubernetes deployment the same, by default. So, unless someone wants to customize the config file, there won't be any new config map the next time they deploy.
Regarding useful settings, the Athens image already has that config.dev.toml file baked into it. Is that what you were thinking?
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 mean we may omit enabled
switch completely.
The reason is that I suppose config.dev.toml
serves only for documentation purposes, and it contains only values that are Athens defaults. Bundling this file into the image actually is the same as passing empty config.
If I'm wrong and config.dev.toml
contains some values that are not Athens default, we can simply put them here in the chart default values. This way we also don't need enabled
switch.
For example:
configToml: |
SomeNonDefaultValue = "foo"
So if users want to customize config, they will rewrite configToml
with their value, otherwise they have the same defaults. enabled
switch is not needed in both cases.
What do you think?
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.
@hypnoglow ah, I think I partially see now what you mean. Regarding the defaults, I would say that there are several "layers" of defaults.
There are hard-coded ones like here the embedded config.dev.toml
one, and then the ones in the helm values.yaml
file. To be honest, I doubt the defaults in code match, the toml file and the helm values.yaml file all match up. However, there are reasons why we do have all three layers. I'm happy to explain them if you'd like or that would help 😄
Regarding the overrides, are you saying that the configToml:
key could have just part of a config toml file, and that we would use it to override the one that is embedded in the Athens docker image?
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.
Regarding the defaults, I would say that there are several "layers" of defaults.
However, there are reasons why we do have all three layers.
Ah, it's a bit clearer now, thanks for explanations. However, don't those multiple layers add unnecessary complexity to the overall Athens configuration? BTW I guess it is going a bit offtopic. If you believe we should stick to those "layers" for now and keep Helm chart in accordance, we should go with it then.
Regarding the overrides, are you saying that the configToml: key could have just part of a config toml file, and that we would use it to override the one that is embedded in the Athens docker image?
Yes, exactly. My point was that we can use default config options directly in helm values that will always override embedded config.
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.
However, don't those multiple layers add unnecessary complexity to the overall Athens configuration?
@hypnoglow I personally think they do, but there is a need for folks to customize configuration at each of the levels, and we have the defaults in place to show where to customize configuration.
For example, the Helm values.yaml
is there show some examples of how folks can customize their Kubernetes deployment. Those fields get translated to environment values passed to Athens, however, so the configuration mechanism doesn't change from other deployments of Athens, for what that's worth 😄 .
If you believe we should stick to those "layers" for now and keep Helm chart in accordance, we should go with it then.
I do think we need to stick with the layers, and I'd like to see them documented (I just created an issue for that, and I'm planning to do it today).
I agree that ideally the helm values.yaml file should be up to date with the config.dev.toml and the hard-coded defaults, and we can try to do that, but inevitably they will become out of sync. I am going to try to write documentation clear enough that folks can both understand where the layers are, how to read each one (config.toml, values.yaml, etc...), and that they may diverge.
My point was that we can use default config options directly in helm values that will always override embedded config.
The merging option is interesting, but wouldn't someone set environment variables or provide a values.yaml
file if they want to override just part of the config.toml
file? This PR is intended to allow people to override everything at once.
Let me know what you think, and thank you for the discussion!
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.
@arschles Thank you for your efforts on improving documentation.
I believe that we can try to reduce layers complexity, somewhat like this:
- Standalone app can run with defaults (should be documented) with the possible custom
config.toml
or environment variables provided by user. - Docker should run with defaults or probably with minimal default
config.toml
with only docker-specific options tuned, if it is even required. Custom configuration should be provided asconfig.toml
-volume or environment variables by user. - Helm Chart and other Kubernetes deployment builds on top of the Docker image, but with default configuration tuned with k8s-specific values (only if required), present in
values.yaml
or similar and allowing to be customized by user.
This way users will always know that they get the same configuration, whether they running Athens standalone, in Docker or in Kubernetes.
The merging option is interesting, but wouldn't someone set environment variables or provide a values.yaml file if they want to override just part of the config.toml file? This PR is intended to allow people to override everything at once.
I didn't mean merging. To recap what I suggested:
-
If we use
enabled
-switch approach, then withenabled: false
helm chart runs with Docker-builtinconfig.toml
and custom configuration can be passed only with environment variables. Withenabled: true
we completely override Docker-builtinconfig.toml
file with contents indata
key. In this approach,configToml.data
contains no default configuration, and users settingenabled: true
will not know that this will override default Docker-builtin config completely and consequences. -
If we don't use
enabled
switch, thenconfigToml
contents ALWAYS override Docker-builtinconfig.toml
, so users don't even have to know about that config file built in the docker image. This way we can embed default configuration for Helm deployment directly intovalues.yaml
, and it always get injected. If user needs to change it, they can add/remove configuration options in their customvalues.yaml
-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.
@hypnoglow you're welcome
Standalone app can run with defaults (should be documented) with the possible custom config.toml or environment variables provided by user.
This is working now. You can use the -config_file
flag (we will need to document that)
Docker should run with defaults or probably with minimal default config.toml with only docker-specific options tuned, if it is even required. Custom configuration should be provided as config.toml-volume or environment variables by user.
We would need to them remove the current config.toml
file from the docker image, which will cause backward compatibility issues. And that is the root problem we will need to solve.
Would it be helpful for Athens to have an environment variable like CONFIG_FILE
:
- Set to
/path/to/file.toml
to tell Athens to read from a specific config file. you can already pass it the-config_file
flag, but that is more difficult when running in a docker container - Set to
off
to tell Athens to completely ignore the config file and go with hard-coded defaults and environment variables
Helm Chart and other Kubernetes deployment builds on top of the Docker image, but with default configuration tuned with k8s-specific values (only if required), present in values.yaml or similar and allowing to be customized by user.
We still will need to keep the default config.toml
in the docker image, but we can also make it easier here to mount your own config.toml file or turn it off
What do you think?
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.
(P.S. I'll respond much more quickly this time. I have far less travel this year 😄)
Closing because you can already achieve this with |
Fixes gomods/athens-charts#22
What is the problem I am trying to address?
There's currently no way to add a complete
config.toml
file to an Athens pod(s) running in KubernetesHow is the fix applied?
I added a
configToml
section to thevalues.yaml
file in the Helm chart, where you can setenabled: true
and then set adata
field to a completeconfig.toml
if you want. The chart will then create a newConfigMap
and mount it to/config/config.toml
inside the new pod(s) that get created when youhelm install
Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.
Fixes gomods/athens-charts#22
cc/ @hypnoglow - let me know if this is what you had in mind. Also, I didn't test this out yet, so let me know if it looks right to you!