feat: add local subcommand to compare two chart folders #866
Conversation
|
@bo0tzz maybe we should add an e2e test? |
|
The rest is LGTM. @bo0tzz |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new local subcommand to the helm-diff plugin that allows comparing two local chart directories. The feature enables developers to see differences between chart versions during development and CI/CD workflows by rendering both charts with helm template and showing the resulting manifest differences.
Key changes:
- Implements a new
localsubcommand with comprehensive flag support for chart rendering options - Adds comprehensive test coverage for argument validation and command execution
- Updates documentation to include the new subcommand usage and examples
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/root.go | Registers the new localCmd() subcommand in the main command structure |
| cmd/local.go | Complete implementation of the local subcommand with chart rendering and diffing logic |
| cmd/local_test.go | Test suite covering argument validation and basic execution scenarios |
| README.md | Documentation update adding the local subcommand help text and usage examples |
| func (l *local) run() error { | ||
| manifest1, err := l.renderChart(l.chart1) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err) |
There was a problem hiding this comment.
Error messages should start with lowercase letters to follow Go conventions.
| return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err) | |
| return fmt.Errorf("failed to render chart %s: %w", l.chart1, err) |
|
|
||
| manifest2, err := l.renderChart(l.chart2) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) |
There was a problem hiding this comment.
Error messages should start with lowercase letters to follow Go conventions.
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) | |
| return fmt.Errorf("failed to render chart %s: %w", l.chart2, err) |
| args := []string{"template", l.release, chartPath} | ||
| args = append(args, flags...) | ||
|
|
||
| cmd := exec.Command(os.Getenv("HELM_BIN"), args...) |
There was a problem hiding this comment.
Using os.Getenv("HELM_BIN") without validation could allow execution of arbitrary commands. Consider validating the HELM_BIN path or providing a default fallback.
| cmd := exec.Command(os.Getenv("HELM_BIN"), args...) | |
| helmBin := os.Getenv("HELM_BIN") | |
| if helmBin == "" { | |
| helmBin = "helm" | |
| } | |
| cmd := exec.Command(helmBin, args...) |
|
@yxxhero I'd be down for e2e tests, but I don't think the project has any yet right? How would you like those to look? |
|
@bo0tzz it will be better if we add e2e test. this is a new feature. thanks so much. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
| return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err) | ||
| } | ||
|
|
||
| manifest2, err := l.renderChart(l.chart2) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) |
There was a problem hiding this comment.
Error messages should start with lowercase letters following Go conventions and consistency with other error messages in the codebase.
For example, change:
"Failed to render chart %s: %w"to"failed to render chart %s: %w"
This matches the pattern used in cmd/upgrade.go:292 and cmd/helm3.go:31-41.
| return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err) | |
| } | |
| manifest2, err := l.renderChart(l.chart2) | |
| if err != nil { | |
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) | |
| return fmt.Errorf("failed to render chart %s: %w", l.chart1, err) | |
| } | |
| manifest2, err := l.renderChart(l.chart2) | |
| if err != nil { | |
| return fmt.Errorf("failed to render chart %s: %w", l.chart2, err) |
|
|
||
| manifest2, err := l.renderChart(l.chart2) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) |
There was a problem hiding this comment.
Error message should start with a lowercase letter following Go conventions and consistency with other error messages in the codebase.
For example, change:
"Failed to render chart %s: %w"to"failed to render chart %s: %w"
This matches the pattern used in cmd/upgrade.go:292 and cmd/helm3.go:31-41.
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) | |
| return fmt.Errorf("failed to render chart %s: %w", l.chart2, err) |
| manifest1, err := l.renderChart(l.chart1) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err) | ||
| } | ||
|
|
||
| manifest2, err := l.renderChart(l.chart2) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to render chart %s: %w", l.chart2, err) | ||
| } |
There was a problem hiding this comment.
The renderChart method is called twice (for chart1 and chart2), but it reads from stdin when -f - is used (line 169). Since stdin can only be read once, the second call to renderChart will fail or receive empty input.
This is different from the upgrade command where renderChart is only called once. For the local command, you would need to either:
- Read stdin once before calling
renderChartand pass the content to both calls, or - Document that stdin values (
-f -) are not supported for this command, or - Reuse the same temp file for both chart renders
Consider refactoring to handle stdin reading outside of the loop or before multiple renderChart calls.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
2 participants
This adds a new
localsubcommand that diffs two local chart directories. My intended usecase is mainly for development and CI/CD, to see what effects a change has.This code was written entirely by an LLM; if that is a dealbreaker, feel free to just close this PR outright.
If there is appetite for it, I'd like to next build on top of this with a
gitsubcommand that diffs two git references against eachother (by checking them out to a temporary folder and then runninglocalagainst them).