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 extraEnvVars in helm chart #582

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

callumforrester
Copy link
Contributor

Fixes #581

extraEnvVars was being treated as a string in some places and a list in others. This PR makes it a list everywhere, any values files that look like this should be updated to remove the |:

extraEnvVars: |
  - name: BEAMLINE
    value: i04

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.86%. Comparing base (6d0b50f) to head (0bda41b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #582   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files          40       40           
  Lines        1793     1793           
=======================================
  Hits         1665     1665           
  Misses        128      128           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DominicOram
Copy link
Contributor

DominicOram commented Aug 2, 2024

Maybe naive question but is possible to have written a test that would have caught this? For hyperion we occasionally run our deploy script into /tmp for testing. Maybe a job that deploys the latest version into the central k8s cluster?

@callumforrester
Copy link
Contributor Author

Yes, but I need to look into it further: #559

@DiamondJoseph
Copy link
Contributor

I think this was required for other edge cases where we're injecting something in values.yaml into extraEnvVars as part of the tpl call? Or else maybe if extraEnvVars is empty?

@callumforrester
Copy link
Contributor Author

Hmm, it may have been needed for rabbitmq password injection?

@DiamondJoseph
Copy link
Contributor

Looking at i22,

extraEnvVars: |
    - name: SCRATCH_AREA
      value: {{ .Values.scratch.containerPath }}
    - name: RABBITMQ_PASSWORD
      valueFrom:
        secretKeyRef:
          name: rabbitmq-password
          key: rabbitmq-password

Has this been checked if it worked with the {{.Values.scratch.containerPath}} injection? Or are we going to replicate the value here?

@callumforrester
Copy link
Contributor Author

With #520, the $SCRATCH_AREA variable is no longer needed, I assume the tpl was because of $RABBITMQ_PASSWORD

@callumforrester callumforrester force-pushed the 581-extra-env-vars-broken branch from 76d84b8 to 0bda41b Compare August 8, 2024 09:34
@callumforrester
Copy link
Contributor Author

@DiamondJoseph and @DominicOram are we happy for this to go in?

@callumforrester callumforrester merged commit 5351741 into main Aug 8, 2024
24 checks passed
@callumforrester callumforrester deleted the 581-extra-env-vars-broken branch August 8, 2024 12:16
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.

Extra env vars cause helm to crash
3 participants