fix: file target bug with searchpattern and matchpattern#7614
fix: file target bug with searchpattern and matchpattern#7614josill wants to merge 5 commits intoupdatecli:mainfrom
Conversation
olblak
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request addresses an inconsistent behavior in the file target when using searchpattern: true with matchpattern, where no files match the content pattern. The PR also includes unrelated changes to add new pipeline commands.
Changes:
- Fixed file target to return an error instead of silently skipping when
searchpattern: trueis used withmatchpatternand all files are filtered out - Added new
pipeline applyandpipeline diffcommands as alternatives to the existingapplyanddiffcommands - Deprecated existing
applyanddiffcommands with warning messages
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/plugins/resources/file/target.go | Changed to return error instead of SKIPPED result when no files match criteria with SearchPattern enabled |
| pkg/plugins/resources/file/target_test.go | Added test cases for the bug fix and updated test infrastructure to support SearchPattern tests |
| cmd/pipeline.go | New parent command for pipeline subcommands |
| cmd/pipeline_apply.go | New command implementing pipeline apply functionality |
| cmd/pipeline_diff.go | New command implementing pipeline diff functionality |
| cmd/apply.go | Added deprecation warning and updated Short description |
| cmd/diff.go | Added deprecation warning and updated Short description |
| cmd/root.go | Integrated new pipeline commands into switch cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update mockedContents to use temp directory paths | ||
| updatedContents := make(map[string]string) | ||
| for fileName, content := range tt.mockedContents { | ||
| updatedContents[filepath.Join(tempDir, fileName)] = content | ||
| } | ||
| tt.mockedContents = updatedContents | ||
| // Update wantedContents to use temp directory paths | ||
| if tt.wantedContents != nil { | ||
| updatedWantedContents := make(map[string]string) | ||
| for fileName, content := range tt.wantedContents { | ||
| updatedWantedContents[filepath.Join(tempDir, fileName)] = content | ||
| } | ||
| tt.wantedContents = updatedWantedContents | ||
| } |
There was a problem hiding this comment.
The test setup mutates the test table struct (tt) directly by modifying tt.mockedContents and tt.wantedContents. This is problematic because it can lead to unexpected behavior and makes tests harder to understand. Instead, create new local variables for the modified paths and use those when setting up the test. This also avoids potential issues if tests are run in parallel in the future.
| package cmd | ||
|
|
||
| import ( | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| var ( | ||
| pipelineCmd = &cobra.Command{ | ||
| Use: "pipeline", | ||
| Short: "pipeline executes specific pipeline tasks such as diff or apply", | ||
| } | ||
| ) |
There was a problem hiding this comment.
This PR includes unrelated changes to add new pipeline commands (pipeline apply and pipeline diff) and deprecate existing commands. These changes should be in a separate PR as they are not related to the file target bug fix described in the PR title and description. The PR should focus solely on fixing the file target bug with searchpattern and matchpattern.
| // Create the files that will be matched by the pattern | ||
| for fileName := range tt.files { | ||
| filePath := filepath.Join(tempDir, fileName) | ||
| if err := os.WriteFile(filePath, []byte(""), 0644); err != nil { |
There was a problem hiding this comment.
| if err := os.WriteFile(filePath, []byte(""), 0644); err != nil { | |
| if err := os.WriteFile(filePath, []byte(""), 0600); err != nil { |
…ecli#7506) * fix(terragrunt): add missing username for token authentication Fix token-only authentication by providing username default for go-git. PR updatecli#7359 added token support but token-only auth fails because go-git requires non-empty username for HTTP BasicAuth. This fix adds: - Username *string field to Spec with oauth2 default - getUsername() method matching GitHub SCM plugin behavior - Updated manifest template to include username field - Test coverage for getUsername() Fixes token authentication added in updatecli#7359 * fix(terragrunt):Fixes test failure introduced by adding username support * fix(terragrunt): only include username when token is configured * fix(terragrunt): return empty username when token not configured --------- Co-authored-by: Olivier Vernin <[email protected]>
…#7571) Made with ❤️️ by updatecli Co-authored-by: updateclibot[bot] <92153806+updateclibot[bot]@users.noreply.github.com>
Co-authored-by: Olivier Vernin <[email protected]>
josill
force-pushed
the
fix/file-target-bug
branch
from
9aed10e to
43ea63d
Compare
Fix #1860
Fix inconsistent behavior when using
searchpattern: truewithmatchpatternwhere no files match the content pattern.Changes
searchpattern: trueis used withmatchpatternand all files are filtered out (no matches), the target now returns an error instead of silently skippingsearchpattern: falseandsearchpattern: truenow fail consistently when no files match the patternTest
To test this pull request, you can run the following commands:
Additional Information
Tradeoff
The change modifies behavior for users who relied on the previous silent skip when no files matched with
searchpattern: true. However, this was inconsistent with the behavior whensearchpattern: false, and the issue maintainer confirmed this was a bug that should be fixed.Potential improvement
A future enhancement could add a
matchatleastparameter to allow specifying a minimum number of files that must match before the operation succeeds, providing more flexibility for users who need partial matching scenarios.