Uh oh!
There was an error while loading. Please reload this page.
Remove duplicates from the list of rendered files returned by ReplaceWithRendered - #2
Merged
Merged
Conversation
'helm template' writes to files manifest by manifest, appending to existing files as needed: https://github.com/helm/helm/blob/v3.2.1/pkg/action/action.go#L192-L210 When helmfile feeds the list of files as resources to load to kustomize, this can lead to an error like "may not add resource with an already registered id" if duplicates aren't removed.
mumoshu
approved these changes
May 29, 2020
mumoshu
left a comment
Contributor
There was a problem hiding this comment.
Ah, awesome!
So I see Helm does render rbac.yaml multiple times at the same path as we need multiple lines of wrote datadog/templates/rbac.yaml. The magic is that helm seems to duplicate rbac.yaml by merging all the writes to rbac.yaml.
That being said, this patch is absolutely correct. LGTM. Thank you so much for fixing it!
yxxhero added a commit
that referenced
this pull request
Aug 2, 2026
Addresses the gaps identified in review of the empty-render fix (#206): 1. replace.go: the empty-render branch no longer early-returns. It removes the content dirs and temp output, then falls through so the Chart.yaml `dependencies` field and Chart.lock/requirements.lock cleanup still run. Without this, an empty-rendering chart that declares dependencies would leave Chart.yaml referencing subcharts whose charts/ dir was removed, causing a downstream "found in Chart.yaml, but missing in charts/ directory" error. Also fixes %v -> %w wrapping and adds path context. 2. chartify.go: skip the kustomize Patch step when no resources were rendered (`len(generatedManifestFiles) == 0`). Previously an empty render combined with JsonPatches/StrategicMergePatches/Transformers fed kustomize an empty resource list plus patches, failing with "no resource matches strategic merge patch ...". An empty render is now a no-op success even when patches are configured (there is simply nothing to patch). Tests: - TestEmptyRenderCleansChartDependencies: verifies a chart that declares a dependency and renders nothing still produces a chartifiable output whose Chart.yaml has no dependencies and templates empty. Proven to fail without fix#1. - integration "empty render with patch": exercises the StrategicMergePatches path on the empty chart; proven to fail without fix#2. - testdata/chart_patch/configmap.emptychart.strategic.yaml + empty snapshot.
yxxhero added a commit
that referenced
this pull request
Aug 2, 2026
Addresses the gaps identified in review of the empty-render fix (#206): 1. replace.go: the empty-render branch no longer early-returns. It removes the content dirs and temp output, then falls through so the Chart.yaml `dependencies` field and Chart.lock/requirements.lock cleanup still run. Without this, an empty-rendering chart that declares dependencies would leave Chart.yaml referencing subcharts whose charts/ dir was removed, causing a downstream "found in Chart.yaml, but missing in charts/ directory" error. Also fixes %v -> %w wrapping and adds path context. 2. chartify.go: skip the kustomize Patch step when no resources were rendered (`len(generatedManifestFiles) == 0`). Previously an empty render combined with JsonPatches/StrategicMergePatches/Transformers fed kustomize an empty resource list plus patches, failing with "no resource matches strategic merge patch ...". An empty render is now a no-op success even when patches are configured (there is simply nothing to patch). Tests: - TestEmptyRenderCleansChartDependencies: verifies a chart that declares a dependency and renders nothing still produces a chartifiable output whose Chart.yaml has no dependencies and templates empty. Proven to fail without fix#1. - integration "empty render with patch": exercises the StrategicMergePatches path on the empty chart; proven to fail without fix#2. - testdata/chart_patch/configmap.emptychart.strategic.yaml + empty snapshot. Signed-off-by: yxxhero <aiopsclub@163.com>
yxxhero added a commit
that referenced
this pull request
Aug 2, 2026
* Initial plan * Fix empty render crash: treat no-output helm template as no-op When helm template --output-dir produces an empty directory (e.g. all templates are gated behind a falsy conditional), ReplaceWithRendered now removes the chart's content dirs and returns successfully with an empty file list instead of crashing with an assertion error. Adds test chart testdata/charts/emptychart and integration test 'empty render no op' to cover this scenario. Closes#206 * Complete the empty-render fix: clean deps, skip kustomize, harden tests Addresses the gaps identified in review of the empty-render fix (#206): 1. replace.go: the empty-render branch no longer early-returns. It removes the content dirs and temp output, then falls through so the Chart.yaml `dependencies` field and Chart.lock/requirements.lock cleanup still run. Without this, an empty-rendering chart that declares dependencies would leave Chart.yaml referencing subcharts whose charts/ dir was removed, causing a downstream "found in Chart.yaml, but missing in charts/ directory" error. Also fixes %v -> %w wrapping and adds path context. 2. chartify.go: skip the kustomize Patch step when no resources were rendered (`len(generatedManifestFiles) == 0`). Previously an empty render combined with JsonPatches/StrategicMergePatches/Transformers fed kustomize an empty resource list plus patches, failing with "no resource matches strategic merge patch ...". An empty render is now a no-op success even when patches are configured (there is simply nothing to patch). Tests: - TestEmptyRenderCleansChartDependencies: verifies a chart that declares a dependency and renders nothing still produces a chartifiable output whose Chart.yaml has no dependencies and templates empty. Proven to fail without fix#1. - integration "empty render with patch": exercises the StrategicMergePatches path on the empty chart; proven to fail without fix#2. - testdata/chart_patch/configmap.emptychart.strategic.yaml + empty snapshot. Signed-off-by: yxxhero <aiopsclub@163.com> * Address review: use unversioned `go test` in example commands The SAVE_SNAPSHOT example comments used `go1.25 test`, but this repo's go.mod requires Go 1.26.0+, so `go1.25 test` would fail. Switch to the unversioned `go test` (reviewer's preferred form) on the two empty-render test cases flagged in review. Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yxxhero <aiopsclub@163.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixesroboll/helmfile#1279
'helm template' writes to files manifest by manifest, appending to existing files as needed:
https://github.com/helm/helm/blob/v3.2.1/pkg/action/action.go#L192-L210
When helmfile feeds the list of files as resources to load to kustomize, this can lead to an error like "may not add resource with an already registered id" if duplicates aren't removed.
Minimal example (helm 3.1.3, kustomize 3.6.1, helmfile 0.118.1)