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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: create helm chart for zarf-agent #3678

wants to merge 4 commits into from
+144 −103

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
@netlify Netlify
Copy link

netlify bot commented Apr 15, 2025 •
edited
Loading

✅ Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit b6a9413
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/67feff5c8661370008468d18

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

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

🚀 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.

brandtkeller
brandtkeller requested changes Apr 15, 2025
View reviewed changes
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).

brandtkeller
brandtkeller requested changes Apr 24, 2025
View reviewed changes
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.

packages/zarf-agent/zarf.yaml
- name: zarf-agent
releaseName: zarf-agent
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a hack - but if we modify this - given that the raw chart name has not changed since creation and that the hasher logic is quite old as well link then we should have quite substantial backwards compatibility AND it should function now.

Suggested change
releaseName: zarf-agent
releaseName: zarf-d2db14ef40305397791454e883b26fc94ad9615d

I say we start here to begin - test other E2E functionality while continuing to get feedback on this as the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandtkeller brandtkeller brandtkeller requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zarf-agent custom tolerations
2 participants
@JeffResc @brandtkeller