fix(provenance): add regression tests and fix flag inheritance#1949
Merged
fix(provenance): add regression tests and fix flag inheritance#1949
Conversation
Add comprehensive test fixtures and snapshot tests for provenance output: - Create provenance-advanced fixture based on quickstart-advanced example - Add snapshot test for complex multi-level import provenance - Verify provenance symbols, depth indicators, and line numbers are correct This regression test captures the expected provenance output format including imports, settings, and vars sections with proper source attribution and depth tracking through import chains. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
…hing types PR #1947 introduced a regression where custom commands couldn't define flags that matched existing flags (like --stack), even when the types matched. This broke the quickstart-advanced example's custom commands. Changes: - Custom commands can now "declare" they need a flag that already exists - If the flag exists with the same type, it's inherited (not re-registered) - If the flag exists with a different type, an error is returned - Shorthand collisions for different flag names still error - Duplicates within the same command still error Updated tests to verify type-matching behavior rather than blocking all redefinitions. Removed duplicated provenance-advanced fixture in favor of using examples/quick-start-advanced directly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
size/l
Comprehensive smoke test that verifies: - Global flags work (--help, --version, --verbose) - Terraform commands with --stack work - Custom commands inherit --stack from parent (quickstart-advanced) - List and describe commands work This provides confidence that the flag inheritance fix doesn't break real-world usage patterns. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive snapshot tests for flag handling to memorialize CLI output and prevent regressions. These tests verify: - Custom commands inherit --stack flag from parent commands - Help output correctly shows inherited flags - Built-in commands continue to work with flags - Custom commands can read inherited flag values Tests cover: - tf plan --help (custom command under terraform alias) - terraform provision --help (custom command) - show component --help (custom command with --stack) - terraform --help (built-in parent command) - describe component --help (built-in command) - show component with -s flag (flag value reading) - version command (global flags) - --help flag (global flag) - describe component with --stack and --provenance - list stacks and list components Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
This shell script is superseded by proper snapshot tests in tests/test-cases/flag-inheritance.yaml which: - Run as part of make testacc - Have golden snapshots capturing exact output - Are integrated with the test framework Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This is the use case that originally led to the flag inheritance fix. Custom commands should be able to declare they need --verbose (a global flag) and inherit it without error, as long as the type matches. Added: - New test fixture: tests/fixtures/scenarios/flag-inheritance/ - Custom 'echo info' command with --verbose flag - Custom 'deploy component' command with --stack flag - Tests verifying help shows the flag, and flag value is readable - Snapshot tests capturing the exact output Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
osterman
force-pushed
the
osterman/provenance-regression-fix
branch
from
e16c837 to
9194175
Compare
osterman
changed the title
The flag inheritance fix commit inadvertently deleted the provenance-advanced test fixture. This restores it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
osterman
changed the title
Contributor
📝 WalkthroughWalkthroughRefactors custom command flag validation to use type-aware inheritance checks instead of reserved flag lookups, moves --version flag to local, adds provenance-aware YAML caching logic, introduces UniqueComponents for deduplicated component listings, restructures list configuration with TopLevelListConfig, and adds comprehensive test fixtures and snapshots. Changes
Sequence DiagramssequenceDiagram
participant CC as Custom Command
participant FlagReg as Flag Registry
participant Parent as Parent Command
participant Validator as Type Validator
participant Result as Result
CC->>FlagReg: Declare flag (e.g., --stack: string)
FlagReg->>Validator: Check duplicates in command
Validator-->>FlagReg: OK / Error
FlagReg->>Parent: Check PersistentFlags
Parent-->>FlagReg: Flag exists (type: bool)
FlagReg->>Validator: Type mismatch? string vs bool
Validator-->>FlagReg: Type conflict detected
FlagReg-->>Result: Return ErrReservedFlagName with details
sequenceDiagram
participant CLI as CLI Request
participant Cache as YAML Cache
participant Parser as YAML Parser
participant Config as Config (TrackProvenance)
participant Result as Cached Entry
CLI->>Cache: UnmarshalYAMLFromFileWithPositions(file, provenance=true)
Cache->>Cache: Check cache hit
alt Cache hit + provenance enabled + positions missing
Cache->>Parser: Re-parse with position tracking
Parser->>Result: Parse + extract positions
Cache->>Cache: Update cache entry with positions
else Cache hit + provenance disabled or positions exist
Cache-->>CLI: Return cached entry
end
sequenceDiagram
participant Stacks as Stacks Map
participant Extract as UniqueComponents
participant Seen as Seen Map
participant Enrich as Metadata Enricher
participant Output as Output List
Stacks->>Extract: UniqueComponents(stacksMap, stackPattern)
loop For each component type
Extract->>Extract: Iterate stacks matching pattern
Extract->>Seen: Track component by (type, name)
alt First occurrence
Extract->>Enrich: Build base component entry
Enrich->>Enrich: Extract vars, settings, component_folder
Enrich-->>Extract: Enriched component
else Duplicate
Extract->>Extract: Increment stack count
end
end
Extract-->>Output: Return deduplicated list with stack_count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@tests/fixtures/scenarios/provenance-advanced/components/terraform/vpc/vpc-flow-logs.tf:
- Around line 3-11: The aws_flow_log "default" resource is using an invalid
module output accessor; replace
module.vpc_flow_logs_bucket[0].outputs.vpc_flow_logs_bucket_arn with the direct
output reference module.vpc_flow_logs_bucket[0].vpc_flow_logs_bucket_arn in the
log_destination assignment so Terraform accesses the module output correctly
(leave surrounding logic like count/local.vpc_flow_logs_enabled,
log_destination_type, traffic_type, vpc_id, and tags unchanged).
🧹 Nitpick comments (3)
tests/fixtures/scenarios/provenance-advanced/stacks/schemas/opa/vpc/validate-vpc-component.rego (2)
40-42: Consider usingregex.matchinstead of deprecatedre_match.The
re_matchbuilt-in is deprecated in newer OPA versions. If this fixture should demonstrate current best practices:-errors[vpc_name_regex_error_message] { - not re_match(vpc_name_regex, input.vars.name) +errors[vpc_name_regex_error_message] { + not regex.match(vpc_name_regex, input.vars.name) }If intentionally kept for backward compatibility testing, a comment noting this would help.
17-19: Consider modern Rego imports.Regal suggests using
import rego.v1for modern OPA policies. This bundles future keywords and enables stricter semantics:package atmos -import future.keywords.in +import rego.v1Optional for test fixtures but aligns with current OPA best practices.
tests/fixtures/scenarios/provenance-advanced/components/terraform/vpc/variables.tf (1)
1-209: Comprehensive VPC variable definitions.This fixture covers the full spectrum of VPC configuration options with clear descriptions and appropriate types. The heredoc descriptions for complex variables like
availability_zonesandipv4_cidrsare helpful.Optional: Similar to the flow logs bucket, you could add validation blocks for constrained string values like
vpc_flow_logs_traffic_typeandvpc_flow_logs_log_destination_type.♻️ Optional validation blocks
variable "vpc_flow_logs_traffic_type" { type = string description = "The type of traffic to capture. Valid values: `ACCEPT`, `REJECT`, `ALL`" default = "ALL" + validation { + condition = contains(["ACCEPT", "REJECT", "ALL"], var.vpc_flow_logs_traffic_type) + error_message = "Valid values are: ACCEPT, REJECT, ALL." + } } variable "vpc_flow_logs_log_destination_type" { type = string description = "The type of the logging destination. Valid values: `cloud-watch-logs`, `s3`" default = "s3" + validation { + condition = contains(["cloud-watch-logs", "s3"], var.vpc_flow_logs_log_destination_type) + error_message = "Valid values are: cloud-watch-logs, s3." + } }
tests/fixtures/scenarios/provenance-advanced/components/terraform/vpc/vpc-flow-logs.tf
Outdated
Show resolved
Hide resolved
- Add status indicator (colored dot for TTY, text for piped output)
- Add component_folder field extracted from metadata.component
- Refactor test fixtures to use shared mock component library
- Delete unused provenance-advanced fixture
- Update default sort to stack:asc,component:asc for deterministic output
The status field shows:
- TTY: Colored dot (green=enabled, red=locked, gray=disabled)
- Piped: Semantic text ("enabled", "disabled", "locked")
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This PR fixes the list components command to show unique component definitions (deduplicated across stacks) instead of component instances. Changes: - Add UniqueComponents() function with --stack filter support - Introduce new config namespace: list.components and list.instances - Maintain backward compatibility with components.list.columns - Add explicit TTY/non-TTY tests for status indicator - Update quick-start-advanced example with new config structure The list components command now shows: - Component name - Component type (terraform, helmfile, packer) - Number of stacks using the component Users can filter by stack pattern: atmos list components --stack "*-dev" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…efine it The --version flag was registered as a persistent global flag, causing it to be inherited by all subcommands. This prevented custom commands from defining their own --version flag (e.g., `atmos install --version 1.0.0`). Changes: - Remove --version from GlobalOptionsBuilder persistent flags - Register --version as a local flag on RootCmd only - Update flag conflict detection to only check PersistentFlags() and InheritedFlags(), not local Flags() on parent commands - Add unit test TestCustomCommand_VersionFlagAllowed - Add integration test for custom command with --version flag The --use-version flag remains persistent as it's needed for re-exec on any subcommand. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update test expectations after d021669 changed list components to show unique component definitions instead of per-stack instances: - Stack filter is no longer applied (use list instances for per-stack filtering) - Default columns changed from 6 to 3 (Component, Type, Stacks) - Config path changed from Components.List to List.Components - Default sorter changed to sort by component only Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
|
@coderabbitai full review please |
Contributor
|
@aknysh, I'll conduct a full review of the changes in this PR. 🧠 Learnings used✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1949 +/- ##
==========================================
+ Coverage 74.13% 74.17% +0.03%
==========================================
Files 772 772
Lines 69744 69847 +103
==========================================
+ Hits 51708 51806 +98
- Misses 14611 14617 +6
+ Partials 3425 3424 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/cmd_utils.go (2)
115-256: Potential panic: config flag can collide with flags already added tocustomCommand(e.g., auto-added--identity).
Right now validation + “skip registration” only looks atparentCommand.{PersistentFlags,InheritedFlags}. If a config declaresidentity(or any other pre-seeded flag oncustomCommand), the registration loop can still hit a pflag “flag redefined” panic.Proposed fix (check customCommand first, then parent chain).
@@ - seen := make(map[string]bool) + seen := make(map[string]bool) for _, flag := range commandConfig.Flags { @@ - existingFlag := parentCommand.PersistentFlags().Lookup(flag.Name) - if existingFlag == nil { - existingFlag = parentCommand.InheritedFlags().Lookup(flag.Name) - } + // Check the custom command itself first (it already has flags like --identity). + existingFlag := customCommand.PersistentFlags().Lookup(flag.Name) + if existingFlag == nil { + existingFlag = parentCommand.PersistentFlags().Lookup(flag.Name) + } + if existingFlag == nil { + existingFlag = parentCommand.InheritedFlags().Lookup(flag.Name) + } @@ - // Flag doesn't exist yet - validate shorthand for new flags only. + // Flag doesn't exist yet - validate shorthand for new flags only. if flag.Shorthand != "" { @@ - existingByShorthand := parentCommand.PersistentFlags().ShorthandLookup(flag.Shorthand) + existingByShorthand := customCommand.PersistentFlags().ShorthandLookup(flag.Shorthand) + if existingByShorthand == nil { + existingByShorthand = parentCommand.PersistentFlags().ShorthandLookup(flag.Shorthand) + } if existingByShorthand == nil { existingByShorthand = parentCommand.InheritedFlags().ShorthandLookup(flag.Shorthand) } @@ // Process and add flags to the command. // Skip flags that are inherited from parent command chain. for _, flag := range commandConfig.Flags { - existingFlag := parentCommand.PersistentFlags().Lookup(flag.Name) + // Skip flags already present on the custom command (e.g., auto-added --identity) + // and inherited/persistent flags from the parent chain. + existingFlag := customCommand.PersistentFlags().Lookup(flag.Name) if existingFlag == nil { existingFlag = parentCommand.InheritedFlags().Lookup(flag.Name) } if existingFlag != nil { // Flag exists and type was validated above - skip registration (inherit). continue }
115-137: Comment punctuation may tripgodot(multi-line//blocks).
Some lines in the added comment blocks end with commas or no period. Ifgodotis strict per-line in this repo, add trailing periods to each// ...line.Also applies to: 181-184
cmd/list/components.go (1)
110-116: Config path inconsistency in tab completion.
columnsCompletionForComponentsreferencesatmosConfig.Components.List.Columns(old path), whilegetComponentColumnsat lines 279-290 usesatmosConfig.List.Components.Columns(new path). Consider aligning both to the new config path.Suggested fix
// Extract column names from atmos.yaml configuration. - if len(atmosConfig.Components.List.Columns) > 0 { + if len(atmosConfig.List.Components.Columns) > 0 { var columnNames []string - for _, col := range atmosConfig.Components.List.Columns { + for _, col := range atmosConfig.List.Components.Columns { columnNames = append(columnNames, col.Name) } return columnNames, cobra.ShellCompDirectiveNoFileComp }
🤖 Fix all issues with AI agents
In @pkg/list/extract/components.go:
- Around line 190-237: The stackPattern matching in UniqueComponents currently
uses filepath.Match which is OS-dependent; normalize stackName to use forward
slashes and switch to path.Match to ensure consistent, slash-separated pattern
behavior across platforms: before calling filepath.Match replace backslashes in
stackName with "/" (or canonicalize both stackName and stackPattern to use "/")
and then use path.Match (or call path.Match on the normalized values) instead of
filepath.Match; update the matching logic in UniqueComponents (the block that
handles stackPattern and matched/err) to operate on the normalized strings so
Windows backslashes don't break glob matching.
🧹 Nitpick comments (4)
pkg/list/extract/metadata.go (1)
17-23: Consider caching the TTY check to avoid repeated allocations.Creating
terminal.New()on every call could be inefficient when processing many instances. Based on learnings from this codebase, caching terminal detection at package initialization is the preferred pattern.♻️ Suggested refactor
+var isTTYCached = sync.OnceValue(func() bool { + term := terminal.New() + return term.IsTTY(terminal.Stdout) +}) + func getStatusIndicator(enabled, locked bool) string { - // Check if stdout is a TTY. - term := terminal.New() - isTTY := term.IsTTY(terminal.Stdout) - - return getStatusIndicatorWithTTY(enabled, locked, isTTY) + return getStatusIndicatorWithTTY(enabled, locked, isTTYCached()) }You'll need to add
"sync"to imports.pkg/utils/yaml_utils.go (1)
799-814: Logic is correct; minor redundancy.The provenance-aware cache re-parse logic works well. Note: the
atmosConfig != nilcheck on line 802 is redundant since lines 780-782 already return an error for nil config. Not a blocker—just a small cleanup opportunity.🔧 Optional: remove redundant nil check
if found { // Cache hit - but check if we need positions and don't have them. // This can happen if the file was first parsed without provenance tracking, // then later requested with provenance enabled. - if atmosConfig != nil && atmosConfig.TrackProvenance && len(positions) == 0 { + if atmosConfig.TrackProvenance && len(positions) == 0 { // Need to re-parse with position tracking. // Force a cache miss to re-parse and update the cache with positions. found = falsepkg/utils/yaml_utils_test.go (1)
1465-1590: Global cache/stat mutations aren’t restored (possible test flakiness).
These tests resetparsedYAMLCacheandparsedYAMLCacheStatsbut don’t restore the previous state, which can leak into other tests in theutilspackage.Example pattern (capture + restore via t.Cleanup).
@@ func TestUnmarshalYAMLFromFileWithPositions_ProvenanceReparse(t *testing.T) { + // Capture old state. + parsedYAMLCacheMu.Lock() + oldCache := parsedYAMLCache + parsedYAMLCacheMu.Unlock() + t.Cleanup(func() { + parsedYAMLCacheMu.Lock() + parsedYAMLCache = oldCache + parsedYAMLCacheMu.Unlock() + }) + // Clear cache to ensure clean state. parsedYAMLCacheMu.Lock() parsedYAMLCache = make(map[string]*parsedYAMLCacheEntry) parsedYAMLCacheMu.Unlock() @@ }cmd/custom_command_flag_conflict_test.go (1)
1207-1508: Optional: reduce repeated fixture/TestKit setup by usingnewCustomCommandTestHelper.
Not blocking, but it would make the new inheritance tests shorter and more uniform.
aknysh
previously approved these changes
Jan 13, 2026
aknysh
dismissed stale reviews from coderabbitai[bot] and themself
via
59c6cba
aknysh
approved these changes
Jan 13, 2026
|
These changes were released in v1.204.0-rc.4. |
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 free
to 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.
what
--stack,--verbose, or other existing flags when the type matches--stackas bool when parent has it as string)why
--stackand--verbosethat are defined elsewherereferences
tests/fixtures/scenarios/provenance-advanced/andtests/fixtures/scenarios/flag-inheritance/tests/test-cases/provenance-snapshots.yamlandtests/test-cases/flag-inheritance.yamlSummary by CodeRabbit
New Features
--versionflags independent of the global version flagatmos list componentscommand shows unique components with stack countsConfiguration
list.components,list.instances, andlist.stackspaths✏️ Tip: You can customize this high-level summary in your review settings.