refactor: consolidate terraform hook wrappers into cmd/terraform/shared
- Dominant language
- Go
- Stars
- 1.4k
- Forks
- 175
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 134
Description
### Describe the Refactor
The terraform-component hook wrappers in `cmd/terraform/utils.go` (`runHooks`, `runHooksWithOutput`, `runHooksOnError`, `runHooksOnErrorWithOutput`) and the new wrappers in `cmd/terraform/generate/hooks.go` (`runGeneratePlanfileHooks`, `runGeneratePlanfileErrorHook`, added in #2497 PR A) implement the same option-construction → `RunCIHooks` plumbing in two places.
Today the duplication exists because the import direction is one-way: `cmd/terraform` imports `cmd/terraform/generate` ([cmd/terraform/terraform.go#L64](https://github.com/cloudposse/atmos/blob/main/cmd/terraform/terraform.go#L64)), so the `generate` package cannot reuse the unexported wrappers in the parent package without creating a cycle.
### Proposed Solution
Move the hook wrappers into a new file `cmd/terraform/shared/hooks.go` and export them:
- `shared.RunHooks(event, cmd, args) error`
- `shared.RunHooksWithOutput(event, cmd, args, output) error`
- `shared.RunHooksOnError(event, cmd, args, cmdErr)`
- `shared.RunHooksOnErrorWithOutput(event, cmd, args, cmdErr, output)`
Then update all four call sites — plan, apply, deploy, generate-planfile — to use the shared functions. Delete the duplicated `runGeneratePlanfileHooks` / `runGeneratePlanfileErrorHook` in `cmd/terraform/generate/hooks.go`.
### Scope Considerations
- `runHooksWithOutput` in `cmd/terraform/utils.go` includes `resolveComponentPath` (only relevant for path-arg commands) and `--verify-plan` flag reading. The generate-planfile wrapper doesn't need either. The consolidated version should keep both call-sites correct — likely by making the path-resolution and verify-plan reads conditional on flag presence (already happens for verify-plan via `_ = cmd.Flags().GetBool("verify-plan")` returning false for commands that don't define it).
- `wirePerComponentHook` in `cmd/terraform/utils.go` does NOT need to move — only plan/apply/deploy use it and they all live in `cmd/terraform`. The generate package doesn't support multi-component flags today.
- The `wasMultiComponentExecution` sentinel and the `runHooksOnErrorWithOutput` package-level var (used for stubbing in deploy-defer-guard tests) need to migrate as well; tests in `cmd/terraform/utils_hooks_test.go` will need to reference the shared package.
### Why this is a follow-up, not part of #2497 PR A
PR A is scaffold for the hook lifecycle integration — small, reviewable, and contained. A cross-package refactor that touches plan / apply / deploy / generate-planfile would inflate the diff and obscure the actual feature being added. Separating the refactor lets it land independently with focused review.
### Acceptance Criteria
- [ ] `cmd/terraform/shared/hooks.go` defines the four exported wrappers.
- [ ] `cmd/terraform/utils.go` no longer defines `runHooks*` (or keeps them as thin shims that delegate to `shared`, marked deprecated).
- [ ] `cmd/terraform/generate/hooks.go` is deleted (its TODO marker is the trigger for this issue).
- [ ] `cmd/terraform/utils_hooks_test.go` tests still pass against the relocated implementations.
- [ ] No new behavior — pure refactor, identical CI semantics.
### References
- TODO marker added in #2497 PR A: `cmd/terraform/generate/hooks.go` lines 25-28.
- Existing wrappers: [`cmd/terraform/utils.go`](https://github.com/cloudposse/atmos/blob/main/cmd/terraform/utils.go#L36-L82).
Contributor guide
Research direction
Start by comparing the wrappers in cmd/terraform/utils.go and cmd/terraform/generate/hooks.go, then inspect the plan, apply, deploy, and generate-planfile call sites. Run cmd/terraform/utils_hooks_test.go while tracking the sentinel and stubbed error wrapper; done means the shared wrappers serve all four paths and the tests pass with identical CI behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, terraform
- Domain
- cli, devops
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100