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

fix: Enable Extra Env in Chart #490

Closed
wants to merge 6 commits into from

Conversation

3schwartz
Copy link
Contributor

Related Tickets & Documents

In PR #487, support was added to provide additional environment variables to the operator.

However, it is currently not possible to specify these variables via the Helm Chart.

Changes

This PR enables the ability to provide extra environment variables to the operator from the Helm Chart.

@coveralls
Copy link
Collaborator

coveralls commented Dec 16, 2024

Pull Request Test Coverage Report for Build 12360249215

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.214%

Totals Coverage Status
Change from base Build 12359248301: 0.0%
Covered Lines: 1021
Relevant Lines: 1036

💛 - Coveralls

@3schwartz
Copy link
Contributor Author

Hi @ekampf

I realized that in PR #487, I forgot to make the new settings configurable through the Helm chart.
Would you be able to review this?

Should I also bump the chart version as part of this change?

@ekampf
Copy link
Contributor

ekampf commented Dec 16, 2024

@3schwartz yeah missed that too...
yes Please up the chart version and I'll release another version

@ekampf ekampf changed the title chore: Enable Extra Env in Chart fix: Enable Extra Env in Chart Dec 16, 2024
@3schwartz
Copy link
Contributor Author

@3schwartz yeah missed that too... yes Please up the chart version and I'll release another version

@ekampf Done

One of the integration tests is failing with the error remote_network_id missing, but I’m not sure why my changes would cause this 🤔

The chart is currently referencing the latest image. Once this PR is merged, will both the new chart and the updated image be available with the changes from the prior PR, or is a release action required?

@@ -70,6 +70,10 @@ spec:
- name: TWINGATE_REMOTE_NETWORK_NAME
value: {{ .Values.twingateOperator.remoteNetworkName }}
{{- end }}
{{- range $key, $value := .Values.extraEnvVars }}
- name: {{ $key }}
Copy link
Contributor

@ekampf ekampf Dec 16, 2024

Choose a reason for hiding this comment

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

Do we want to do that or just keep the k8s structure (name, value) and do something like {{- toYaml .Values.extraEnvVars | nindent 8 }} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking at whats common in other charts and it seems most do something like:

## extraEnvVars:
##   - name: FOO
##     value: "bar"

@ekampf
Copy link
Contributor

ekampf commented Dec 16, 2024

@3schwartz closing this in favor of #491

@ekampf ekampf closed this Dec 16, 2024
@3schwartz 3schwartz deleted the extra-env-in-chart branch December 17, 2024 05:36
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.

3 participants