fix(chart): allow metadata and additional labels#103
fix(chart): allow metadata and additional labels#103Leundai wants to merge 1 commit intosourcebot-dev:mainfrom
Conversation
WalkthroughThe Helm chart for sourcebot is updated to support additional labels and global metadata configuration. Two new value paths are introduced ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
charts/sourcebot/tests/basic_test.yaml (1)
70-90: Add a collision case to lock label precedence.These two tests only prove each source works in isolation. Please add a case where
additionalLabels.componentandsourcebot.additionalLabels.componentare both set to different values so regressions in the merge order are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/sourcebot/tests/basic_test.yaml` around lines 70 - 90, Add a new test case in the same file that sets both additionalLabels.component and sourcebot.additionalLabels.component to different values to verify merge precedence; specifically create an "it" entry (e.g., "should prefer sourcebot.additionalLabels over root additionalLabels") that uses values ../values.lint.yaml, sets additionalLabels.component: rootVal and sourcebot.additionalLabels.component: sourceVal, and asserts the final value at metadata.labels.component in template deployment.yaml equals the expected winner (state which should win in your chart, e.g., sourceVal) so any regression in merge order is caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/sourcebot/templates/_helpers.tpl`:
- Around line 43-46: The helper uses mergeOverwrite with arguments in the order
(dict) (default dict $.Values.sourcebot.additionalLabels) (default dict
$.Values.additionalLabels) so top-level $.Values.additionalLabels currently wins
on key collisions; either reverse the merge order so
$.Values.sourcebot.additionalLabels takes precedence (call mergeOverwrite with
$.Values.additionalLabels first then $.Values.sourcebot.additionalLabels) or add
explicit documentation in the helpers/template and chart README describing that
top-level additionalLabels override sourcebot.additionalLabels, and add a unit
test exercising key collisions between $.Values.sourcebot.additionalLabels and
$.Values.additionalLabels to lock the intended behavior; update references to
mergeOverwrite, $additionalLabels, $.Values.sourcebot.additionalLabels, and
$.Values.additionalLabels accordingly.
In `@charts/sourcebot/values.schema.json`:
- Around line 34-36: The schema entry for additionalLabels currently allows any
value type; update the additionalLabels definition to require string values by
changing it from a bare "type":"object" to an object schema that sets
additionalProperties (or patternProperties) to {"type":"string"} and optionally
add propertyNames pattern validation to match Kubernetes label key rules; target
the additionalLabels schema in values.schema.json (used by the sourcebot.labels
helper in _helpers.tpl) so any label value merged via sourcebot.labels is
guaranteed to be a string.
---
Nitpick comments:
In `@charts/sourcebot/tests/basic_test.yaml`:
- Around line 70-90: Add a new test case in the same file that sets both
additionalLabels.component and sourcebot.additionalLabels.component to different
values to verify merge precedence; specifically create an "it" entry (e.g.,
"should prefer sourcebot.additionalLabels over root additionalLabels") that uses
values ../values.lint.yaml, sets additionalLabels.component: rootVal and
sourcebot.additionalLabels.component: sourceVal, and asserts the final value at
metadata.labels.component in template deployment.yaml equals the expected winner
(state which should win in your chart, e.g., sourceVal) so any regression in
merge order is caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d2ff650-9009-45f1-ba9f-343019f3cd17
📒 Files selected for processing (5)
charts/sourcebot/README.mdcharts/sourcebot/templates/_helpers.tplcharts/sourcebot/tests/basic_test.yamlcharts/sourcebot/values.schema.jsoncharts/sourcebot/values.yaml
| {{- $additionalLabels := mergeOverwrite (dict) (default dict $.Values.sourcebot.additionalLabels) (default dict $.Values.additionalLabels) }} | ||
| {{- with $additionalLabels }} | ||
| {{ toYaml . }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '36,46p' charts/sourcebot/templates/_helpers.tpl
sed -n '70,90p' charts/sourcebot/tests/basic_test.yamlRepository: sourcebot-dev/sourcebot-helm-chart
Length of output: 1112
🌐 Web query:
Helm Sprig mergeOverwrite precedence order which map wins
💡 Result:
mergeOverwrite precedence (which map wins) In Helm templates, mergeOverwrite has argument precedence from right to left: later maps (to the right) overwrite earlier maps (to the left). So for: {{- $out := mergeOverwrite $dst $src1 $src2 -}} - For any conflicting key, $src2 wins. - If a key exists in $src2 and $src1, $src2’s value is used. - If a key does not exist in $src2 but exists in $src1, $src1’s value is used. - If a key exists only in $dst, $dst’s value remains. Example from Helm docs (key collisions): - dest: { default: "default", src: overwritten?, key: true } - src1: { key: false, name: value2 } - src2: { overwrite: overwritten, key: false, name: value1? } Helm’s documented result shows the overwriting effect: the merged value uses right-to-left precedence (e.g., default stays "default", key becomes the value from the rightmost map, and overwrite comes from the rightmost map). Note: mergeOverwrite performs a deep merge (nested maps are merged recursively), but it is not a deep copy; merged nested objects share references. Practical rule of thumb - Use mergeOverwrite $base $overrides1 $overrides2 when you want $overrides2 to win over $overrides1 and $base.
Citations:
- 1: https://helm.sh/docs/chart_template_guide/function_list
- 2: https://helm.sh/docs/chart_template_guide/function_list/
- 3: https://github.com/Masterminds/sprig/blob/master/dict.go
🏁 Script executed:
#!/bin/bash
# Search for collision test cases in test files
find charts -name "*test*.yaml" -o -name "*test*.yml" | xargs grep -l "additionalLabels" 2>/dev/null | head -5Repository: sourcebot-dev/sourcebot-helm-chart
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check full test file to see all label-related tests
cat charts/sourcebot/tests/basic_test.yaml | grep -A 15 "it.*label"Repository: sourcebot-dev/sourcebot-helm-chart
Length of output: 1279
🏁 Script executed:
#!/bin/bash
# Check for comments or documentation around the merge logic
cat charts/sourcebot/templates/_helpers.tpl | head -60Repository: sourcebot-dev/sourcebot-helm-chart
Length of output: 1874
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about additionalLabels precedence
rg -i "additionalLabels|merge.*order|precedence" charts/ README* CHANGELOG* --type yaml --type md -B 2 -A 2 2>/dev/null | head -50Repository: sourcebot-dev/sourcebot-helm-chart
Length of output: 2428
🏁 Script executed:
#!/bin/bash
# Check if there are any other test files that might test collision scenarios
find charts -name "*.yaml" -o -name "*.yml" | xargs grep -l "sourcebot.additionalLabels.*additionalLabels\|additionalLabels.*sourcebot.additionalLabels" 2>/dev/nullRepository: sourcebot-dev/sourcebot-helm-chart
Length of output: 62
Verify the merge order for overlapping label keys.
The merge order is confirmed: mergeOverwrite applies arguments left-to-right, so additionalLabels (top-level) wins over sourcebot.additionalLabels on key collisions. However, the documentation describes both paths identically ("Add extra labels to all resources"), implying they should be treated equally. This precedence is undocumented in the code, and there is no test case covering key collisions between the two paths. If the sourcebot.* path should remain authoritative (as a chart-specific override), the merge order should be reversed, or the design intent should be explicitly documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/sourcebot/templates/_helpers.tpl` around lines 43 - 46, The helper
uses mergeOverwrite with arguments in the order (dict) (default dict
$.Values.sourcebot.additionalLabels) (default dict $.Values.additionalLabels) so
top-level $.Values.additionalLabels currently wins on key collisions; either
reverse the merge order so $.Values.sourcebot.additionalLabels takes precedence
(call mergeOverwrite with $.Values.additionalLabels first then
$.Values.sourcebot.additionalLabels) or add explicit documentation in the
helpers/template and chart README describing that top-level additionalLabels
override sourcebot.additionalLabels, and add a unit test exercising key
collisions between $.Values.sourcebot.additionalLabels and
$.Values.additionalLabels to lock the intended behavior; update references to
mergeOverwrite, $additionalLabels, $.Values.sourcebot.additionalLabels, and
$.Values.additionalLabels accordingly.
| "additionalLabels": { | ||
| "type": "object" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' charts/sourcebot/values.schema.json
rg -n -C 3 'additionalLabels|metadata.labels|toYaml' charts/sourcebot/templates charts/sourcebot/values.yamlRepository: sourcebot-dev/sourcebot-helm-chart
Length of output: 13267
Constrain additionalLabels to string values.
The schema for this new top-level object lacks property type validation. Since it's merged into Kubernetes labels via the sourcebot.labels helper (visible in _helpers.tpl line 43), it could pass schema validation with non-string values that would render invalid Kubernetes labels.
Proposed schema tightening
- "additionalLabels": {
- "type": "object"
- },
+ "additionalLabels": {
+ "type": "object",
+ "additionalProperties": { "type": "string" }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "additionalLabels": { | |
| "type": "object" | |
| }, | |
| "additionalLabels": { | |
| "type": "object", | |
| "additionalProperties": { "type": "string" } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/sourcebot/values.schema.json` around lines 34 - 36, The schema entry
for additionalLabels currently allows any value type; update the
additionalLabels definition to require string values by changing it from a bare
"type":"object" to an object schema that sets additionalProperties (or
patternProperties) to {"type":"string"} and optionally add propertyNames pattern
validation to match Kubernetes label key rules; target the additionalLabels
schema in values.schema.json (used by the sourcebot.labels helper in
_helpers.tpl) so any label value merged via sourcebot.labels is guaranteed to be
a string.
1 participant
Summary
global.metadatain the chart values schema.additionalLabelsto the values schema and documented defaults.sourcebot.additionalLabelsvalue while also supporting the root-level value that the chart helper already reads.Why
The chart currently has a strict values schema. That is helpful for catching mistakes, but it also rejects common Helm usage where deployment tooling passes shared metadata through
global.metadata.There is also a mismatch around
additionalLabels:values.yamland the schema expose it undersourcebot.additionalLabels, but the shared label helper reads from root-leveladditionalLabels. That means the documented value can validate without affecting rendered labels, while the value the templates actually read is rejected by schema validation.This change keeps the schema strict for known fields while allowing these two intended extension points.
Validation
helm dependency update charts/sourcebot/helm lint charts/sourcebot/ -f charts/sourcebot/values.lint.yamlhelm unittest charts/sourcebot/ --color/private/tmp/helm-docs charts/sourcebot/helm template test charts/sourcebot/ -f charts/sourcebot/values.lint.yaml --set global.metadata.owner=platform --set additionalLabels.component=sourcebothelm template test charts/sourcebot/ -f charts/sourcebot/values.lint.yaml --set sourcebot.additionalLabels.component=sourcebotSummary by CodeRabbit
New Features
additionalLabelsglobal.metadataDocumentation
Tests