fix(autodiscovery/helmfile): error on invalid ignore spec#7620
Conversation
|
Thank you very much, it's a nice catch which I think we should also fix on the other autodiscovery plugins as they have similar implementation |
olblak
added
bug
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #7461 by making Helmfile autodiscovery fail fast with clear errors when given invalid spec.ignore or spec.only rules, instead of silently discovering 0 items.
Changes:
- Added validation to reject empty matching rules (rules with no
path,repositories, orchartsfields) - Enabled strict mapstructure decoding with
ErrorUnused: trueto catch invalid spec structures - Changed autodiscovery error logging from
InfotoErrorlnand made the function return an error on validation failures
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/plugins/autodiscovery/helmfile/matchingRule.go | Adds Validate() method to check that each matching rule has at least one non-empty field |
| pkg/plugins/autodiscovery/helmfile/matchingRule_test.go | Comprehensive unit tests for the new validation logic |
| pkg/plugins/autodiscovery/helmfile/main.go | Integrates validation in the New() function and enables strict mapstructure decoding |
| pkg/plugins/autodiscovery/helmfile/main_test.go | Integration tests verifying that invalid ignore/only specs are properly rejected |
| pkg/core/pipeline/autodiscovery/main.go | Changes error handling to log errors properly and return non-zero exit code when autodiscovery fails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ | ||
| ErrorUnused: true, | ||
| Result: &s, | ||
| }) |
There was a problem hiding this comment.
The addition of ErrorUnused: true is a breaking change that will reject configurations with unused fields. While this helps catch the issue described in #7461 where users provide incorrectly structured YAML, it could also break existing configurations that have typos or extra fields that were previously silently ignored.
Consider whether this strictness aligns with the project's compatibility policy. An alternative would be to log a warning for unused fields instead of erroring, though this wouldn't solve the original issue of catching the invalid structure.
| func TestNewWithInvalidIgnoreSpec(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| spec Spec | ||
| rootDir string | ||
| expectError bool | ||
| errorMsg string | ||
| }{ | ||
| { | ||
| name: "valid ignore spec should pass", | ||
| spec: Spec{ | ||
| Ignore: MatchingRules{ | ||
| {Repositories: []string{"https://helm.example.com"}}, | ||
| }, | ||
| }, | ||
| rootDir: "testdata", | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "empty ignore rule should fail", | ||
| spec: Spec{ | ||
| Ignore: MatchingRules{ | ||
| {}, | ||
| }, | ||
| }, | ||
| rootDir: "testdata", | ||
| expectError: true, | ||
| errorMsg: "invalid ignore spec", | ||
| }, | ||
| { | ||
| name: "valid only spec should pass", | ||
| spec: Spec{ | ||
| Only: MatchingRules{ | ||
| {Path: "testdata/*.yaml"}, | ||
| }, | ||
| }, | ||
| rootDir: "testdata", | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "empty only rule should fail", | ||
| spec: Spec{ | ||
| Only: MatchingRules{ | ||
| {}, | ||
| }, | ||
| }, | ||
| rootDir: "testdata", | ||
| expectError: true, | ||
| errorMsg: "invalid only spec", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, err := New(tt.spec, tt.rootDir, "", "") | ||
| if tt.expectError { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.errorMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test cases only verify that empty MatchingRule objects are caught by validation, but they don't test the actual issue case from #7461 where a user provides incorrectly structured input like ignore: { repositories: [...] } instead of ignore: [{ repositories: [...] }]. Consider adding a test that passes raw map input (similar to what would come from YAML parsing) to verify that both the decoder and validation work together to catch the reported issue.
olblak
changed the title
2 participants
Fix #7461
Summary
This PR makes Helmfile autodiscovery fail fast on invalid
spec.ignorerules instead of silently discovering 0 items.path,repositories, orcharts) as invalid.Test
go test ./...ignorespec #7461 and confirm it fails with an error (non-zero exit).Additional Information
Checklist
Tradeoff
Now they are rejected with an error to avoid silent failures.
Potential improvement
spec.ignorestructure.