Skip to content

feat: create helm chart for zarf-agent #3678

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JeffResc
Copy link

Description

Converts the zarf-agent manifests to a helm chart, allowing users to customize various values such as affinity and tolerations on the deployment.

Related Issue

Fixes #3620

Checklist before merging

@JeffResc JeffResc requested review from a team as code owners April 15, 2025 17:15
Copy link

netlify bot commented Apr 15, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit d1706d8
🔍 Latest deploy log https://app.netlify.com/projects/zarf-docs/deploys/68409ddf90f24400080da3e4

@JeffResc JeffResc force-pushed the zarf-agent-helm-chart- branch from 5333787 to 3a462f7 Compare April 15, 2025 17:24
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/packager/helm/zarf.go 0.00% 7 Missing ⚠️
Files with missing lines Coverage Δ
src/internal/packager/helm/zarf.go 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

I believe we're missing a baseline values.yaml for the chart (living beside the Chart.yaml) that would serve as a default values definition that agent-values.yaml could then overlay at the Zarf component level.

@JeffResc
Copy link
Author

I believe we're missing a baseline values.yaml for the chart (living beside the Chart.yaml) that would serve as a default values definition that agent-values.yaml could then overlay at the Zarf component level.

I'm not sure there's a need to have any other default values outside of the ones provided by agent-values.yaml. Would an acceptable solution be to move the agent-values.yaml into the chart/values.yaml and remove the valuesFiles reference from zarf.yaml?

@brandtkeller
Copy link
Member

I'm not sure there's a need to have any other default values outside of the ones provided by agent-values.yaml. Would an acceptable solution be to move the agent-values.yaml into the chart/values.yaml and remove the valuesFiles reference from zarf.yaml?

Speaking strictly from a helm chart correctness perspective - the current state will fail helm lint . Much like the zarf-registry package providing a valid chart which we then have values modified at the zarf component layer - I'd opt to follow the same pattern.

@JeffResc
Copy link
Author

Yeah, I agree. There should definitely be a baseline values.yaml in the chart/ directory that I missed. Latest commit moves the values into that file and removes the overrides done at the Zarf package level.

@JeffResc JeffResc force-pushed the zarf-agent-helm-chart- branch from 8d2b3fe to b6a9413 Compare April 16, 2025 00:52
@JeffResc
Copy link
Author

Just realized upgrade testing on this is going to be broken it was using a zarf-generated helm chart before, so the updated release name annotations won't match. The only solution I can think is to patch the existing resources before deploy, but that's hacky and would have to live in the codebase forever, so I don't like that approach. I'm open to ideas on how to solve this more elegantly.

@brandtkeller
Copy link
Member

Just realized upgrade testing on this is going to be broken it was using a zarf-generated helm chart before, so the updated release name annotations won't match. The only solution I can think is to patch the existing resources before deploy, but that's hacky and would have to live in the codebase forever, so I don't like that approach. I'm open to ideas on how to solve this more elegantly.

Upgrade testing is where we expected this to potentially get hairy. I will look at this for my next task and see if anything stands out (likely tomorrow).

Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

Possible path forward - appreciate your patience.

@brandtkeller
Copy link
Member

Looks like some differences between the old/new resources with regards to formatting and tests written to validate an expected state. Let me know if you need assistance running them down.

@brandtkeller
Copy link
Member

Spent a few cycles digging and found that the problem isn't that there is a difference in the expected values - rather a lack of difference when performing the update-creds operation due to the agent release not being found and no error present in that case.

@brandtkeller
Copy link
Member

All checks passing - Believe this is now in a state where we can review the feasibility of the "workaround". I intend to think more about other options that might enable backwards compatibility without needing to keep specific conditional logic.

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.

zarf-agent custom tolerations
2 participants