fix(ci): fire CI hooks per-component in apply --all mode (#2475)#2477
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
📝 WalkthroughWalkthroughResets per-run state at apply start, suppresses global deferred/PostRunE hooks during multi-component (--all) runs, and adds per-component CI-hook invocation for Terraform apply; tests cover wrapper invocation, PostRunE suppression, and exit-code forwarding. ChangesMulti-component apply CI hooks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/terraform/utils_hooks_test.go`:
- Around line 184-196: The test builds a command via newHookTestCmd() which
creates a command with Use: "plan", but the test invokes applyCmd.PostRunE,
causing a mismatch; update the test so the command identity matches apply:
either create a new helper/newHookTestCmd that accepts a use-name and call
newHookTestCmd("apply"), or set cmd.Use = "apply" on the returned command before
calling applyCmd.PostRunE(cmd, ...), ensuring newHookTestCmd(),
applyCmd.PostRunE, and the cmd variable reflect the "apply" command identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1955e1b9-f0a4-4dba-88fa-dc88cc33c577
📒 Files selected for processing (3)
cmd/terraform/apply.gocmd/terraform/utils.gocmd/terraform/utils_hooks_test.go
Addresses CodeRabbit review on PR cloudposse#2477: newHookTestCmd() created a command with Use:"plan" but the test calls applyCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Same fix as PR cloudposse#2477: newHookTestCmd() hardcodes Use:"plan" but the test calls deployCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…2475) Extends the per-component hook pattern from PR cloudposse#2430 (plan --all) to apply --all so each component produces its own CI summary entry instead of a single misattributed entry for the last component. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Addresses CodeRabbit review on PR cloudposse#2477: newHookTestCmd() created a command with Use:"plan" but the test calls applyCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1984411 to
a7781e8
Compare
Same fix as PR cloudposse#2477: newHookTestCmd() hardcodes Use:"plan" but the test calls deployCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/terraform/utils_hooks_test.go (1)
156-198: ⚡ Quick winInitialize
cmd.NewTestKit(t)in these new cmd tests.Please add
cmd.NewTestKit(t)at the start of each new test here so command/root state cleanup is consistent with repo test conventions.As per coding guidelines "
cmd/**/*_test.go: ALWAYS usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)."Also applies to: 203-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/terraform/utils_hooks_test.go` around lines 156 - 198, The tests TestRunCIHooksForApplyComponent_DemoStacks and TestApplyPostRunE_SuppressedWhenMultiComponent need to call cmd.NewTestKit(t) at the start to ensure RootCmd state is reset between tests; add a cmd.NewTestKit(t) invocation as the first statement in each of these test functions (before t.Chdir and before creating cmd via newHookTestCmd()), so command/root flags and args are auto-cleaned for tests that exercise newHookTestCmd(), applyCmd.PostRunE and the wasMultiComponentExecution logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/terraform/utils_hooks_test.go`:
- Around line 156-198: The tests TestRunCIHooksForApplyComponent_DemoStacks and
TestApplyPostRunE_SuppressedWhenMultiComponent need to call cmd.NewTestKit(t) at
the start to ensure RootCmd state is reset between tests; add a
cmd.NewTestKit(t) invocation as the first statement in each of these test
functions (before t.Chdir and before creating cmd via newHookTestCmd()), so
command/root flags and args are auto-cleaned for tests that exercise
newHookTestCmd(), applyCmd.PostRunE and the wasMultiComponentExecution logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a640242-3b5c-49cf-81a5-712501a1d528
📒 Files selected for processing (3)
cmd/terraform/apply.gocmd/terraform/utils.gocmd/terraform/utils_hooks_test.go
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (64.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2477 +/- ##
==========================================
+ Coverage 78.21% 78.24% +0.03%
==========================================
Files 1119 1119
Lines 106243 106266 +23
==========================================
+ Hits 83095 83148 +53
+ Misses 18509 18483 -26
+ Partials 4639 4635 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Same fix as PR cloudposse#2477: newHookTestCmd() hardcodes Use:"plan" but the test calls deployCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Status summary for maintainersCode state: CodeRabbit APPROVED at the current HEAD What's blocking merge:
Re: the inline nitpick about |
Andriy Knysh (aknysh)
left a comment
There was a problem hiding this comment.
thanks thejrose1984
06a3104
into
cloudposse:main
|
These changes were released in v1.220.0-rc.1. |
* fix(ci): fire CI hooks per-component in deploy --all mode (#2476) Extends the per-component hook pattern from PR #2430 (plan --all) and #2475 (apply --all) to deploy --all so each component produces its own CI summary entry instead of a single misattributed entry for the last component. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: clarify exit code 2 label in deploy hook forwarding test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: use deploy command identity in PostRunE suppression test Same fix as PR #2477: newHookTestCmd() hardcodes Use:"plan" but the test calls deployCmd.PostRunE, causing ProcessCommandLineArgs to see the wrong subcommand name in the non-suppressed path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(deploy): add RunE defer-guard regression test Convert runHooksOnErrorWithOutput to a package-level var so tests can stub it, and add TestDeployRunE_DeferGuard covering all four branches of the deploy.go RunE defer-guard contract: non-nil error must fire the global hook only when wasMultiComponentExecution is false, and nil error must never fire it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): repair broken merge from main and refactor tests The previous merge from main left the branch unbuildable: - cmd/terraform/utils.go was missing a closing brace on the deploy PerComponentHook branch, producing a Go syntax error - cmd/terraform/utils_hooks_test.go had deploy and apply test bodies interleaved with overlapping function declarations and orphaned statements Reconstruct the test file to host both the deploy tests (this PR) and the apply tests (brought in from main via #2475), and address open CodeRabbit nitpicks at the same time: - Add a compile-time sentinel for schema.ConfigAndStacksInfo so a field rename surfaces as a build failure - Convert plan/deploy/apply demo-stacks tests and the deploy RunE defer-guard scenarios to table-driven subtests - Document why cmd.NewTestKit(t) cannot be used here (circular import between cmd/terraform and cmd, same workaround the workdir tests use) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor: narrow test changes to deploy scope and fix comment Code review follow-ups: - Restore the linear style of plan/apply demo-stacks tests to match main; these tests came in via merge and aren't part of this PR's scope. Only the deploy demo-stacks test and the deploy RunE defer-guard scenarios are refactored to table-driven subtests, per the CodeRabbit nitpicks for this PR. - Update the per-component hook wiring comment in utils.go to read "plan, deploy, and apply" now that all three branches are wired. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Extends the per-component CI hook pattern from PR #2430 (plan --all) to apply --all, so each component produces its own CI summary entry instead of a single misattributed entry for the last component.
what
why
references
Summary by CodeRabbit
Bug Fixes
Tests