fix: correct label helper and annotation references in job/cronjob te…#100
fix: correct label helper and annotation references in job/cronjob te…#100fernandoataoldotcom wants to merge 8 commits into
Conversation
…mplates - job.yaml: use chart.commonLabels instead of chart.appLabels to match all other templates (adds managed-by, helm.sh/chart, teamowner labels) - job.yaml: fix annotation reference from $.Values.cronJob.annotations to $.Values.job.annotations - cronjob.yaml: replace merge with set for suffixName context to fix label rendering when multiple cronjobs are defined
There was a problem hiding this comment.
Pull request overview
This PR updates Helm chart templates to standardize label/annotation helpers and to correct context handling for Job/CronJob resources so they render consistent metadata across templates.
Changes:
job.yaml: switch fromchart.appLabelstochart.commonLabelsand fix annotations lookup toValues.job.annotations.cronjob.yaml: change howsuffixNameis passed intochart.commonLabelsto address label rendering with multiple cronjobs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| templates/job.yaml | Uses chart.commonLabels for Jobs and corrects annotation source to Values.job.annotations. |
| templates/cronjob.yaml | Adjusts suffixName context passed to chart.commonLabels for CronJobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t mutation Replace `set $ "suffixName"` with `merge (deepCopy $) (dict "suffixName" $name)` in both cronjob.yaml and job.yaml. `set` mutates the root context ($) in place, causing the last job/cronjob suffixName to leak into templates rendered afterward (e.g., Deployment, Service), incorrectly appending the suffix to their app.kubernetes.io/name label.
Assert that chart.commonLabels renders managed-by and helm.sh/chart in Job metadata, and that job.annotations (not cronJob.annotations) appears in Job metadata.annotations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Align with the assertion style used throughout the rest of the test suite. isNotEmpty may not be supported by all helm-unittest versions.
Render two cronjobs and assert each document gets a distinct app.kubernetes.io/name label, covering the deepCopy fix that prevents suffixName from leaking between loop iterations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Map iteration order is non-deterministic, so documentIndex-based assertions can be flaky. Use documentSelector to match documents by metadata.name before asserting labels.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
After replacing set $ with deepCopy, the pod template's .Root no longer carries suffixName, causing pod app.kubernetes.io/name labels to lose the job suffix. Pass $newContext (which includes suffixName) as Root so Job/CronJob metadata and pod labels stay consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…mplates