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

Better handling of PATH env var with initConfig #401

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

vmdude
Copy link
Contributor

@vmdude vmdude commented Jul 2, 2024

what

We got a use-case where we need to defined the PATH environment variable through the .Values.environment node for the statefulset, and we had some issues due to duplicate entries.
When .Values.initConfig is enabled, the chart actually add a PATH environment variable.
The issue is due to how Kubernetes handle duplicate envvars definition, when a referenced key is present in multiple resources, the value associated with the last source will override all previous values.
As .Values.initConfig variable is defined later in the manifest, it will always supersede previously definition, thus our .Values.environment node was bypassed.

This PR will add the following logic:

  • if .Values.initConfig.enabled and no PATH in .Values.environment: add a PATH envvar like before
  • if .Values.initConfig.enabled and PATH in .Values.environment: do not add a PATH envvar as it should use the one defined in .Values.environment

The rest of the logic is untouched.

why

We wanted to be able to define more specifically how the PATH environment variable is presented to the statefulset avoiding any duplicate in the process.

tests

  • I have tested my changes by created several values file for all possible combination regarding PATH and initConfig parameters

references

  • More information about precedence and duplicate env var management [so]

@vmdude vmdude requested a review from a team as a code owner July 2, 2024 12:32
Copy link
Member

@GMartinez-Sisti GMartinez-Sisti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot on 🚀

@GMartinez-Sisti GMartinez-Sisti merged commit a72f86c into runatlantis:main Jul 2, 2024
2 checks passed
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