Skip to content
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

add support for environment variables #102

Closed
wants to merge 1 commit into from

Conversation

vanHavel
Copy link

According to the CoreDNS docs, environment variables can be used in the Corefile. This might make a few things easier to configure, as the server config can be defined in values.yaml and small overrides can be done using environment variables in other values files.

This PR adds supports for environment variables to the CoreDNS deployment.

@vanHavel
Copy link
Author

After creating the PR, I saw that there is already #90 which implements the same feature. The only difference is that #90 uses lists, while this uses maps to represent the env vars.

Personally I find maps to be slightly preferable, as they allow easier overrides, e.g. with --set.

@hagaibarel
Copy link
Collaborator

Hi, thanks for addition, definitely something we should support.

I think your approach will limit the chart to support only key: value type of env vars and not more complex use case, e.g. using valueFrom with a reference to a configmap.

Signed-off-by: Lukas Huwald <dev.lukas.huwald@gmail.com>
@vanHavel
Copy link
Author

vanHavel commented Mar 28, 2023

That is true, I adapted my approach to also allow for complex env specifications, while still keeping the env var keys in a map.

However, #90 does also allow this (with lists), which is more directly following the k8s format of specifying env - I would be happy if either gets accepted.

@hagaibarel
Copy link
Collaborator

I think #90 addresses the env vars issue in a simpler way, let's focus on that one

@hagaibarel hagaibarel closed this Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants