Uh oh!
There was an error while loading. Please reload this page.
feat: add support for chart version - #1
Closed
gmenuel wants to merge 1 commit into
Closed
Conversation
mumoshu
commented
May 29, 2020
Contributor
@gmenuel Oh, wow! Thanks you so much for submitting this Too bad I couldn't notice this before I fixed it myself... My own change seems equivalent to yours, so I was able to double-check the correctness of my work thanks to your PR. I'm closing this as resolved, but please feel free to report any issue/poke me for earlier review, so that this won't happen again! Thanks again for your support |
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.
Currently the chart version is passed to chartify, but it isn't used when fetching the charts.