Repository navigation
feat: Add custom component types for custom commands - #2469
Closed
Ben (Benbentwo) wants to merge 1 commit into
Closed
Ben (Benbentwo) wants to merge 1 commit into
Ben (Benbentwo) wants to merge 1 commit into
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Contributor
|
💥 This pull request now has conflicts. Could you fix it Ben (@Benbentwo)? 🙏 |
Contributor
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ATMOS_OUTPUTS support and parsing for custom-component commands, normalizes hook event names, centralizes hook execution in internal.RunHooks (accepting pre-resolved component data), and routes store-hook output lookups to Terraform state or the ATMOS_OUTPUTS file based on component type. ChangesLifecycle Hooks for Custom Components
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (3)
internal/exec/stack_processor_process_stacks_test.go (2)
882-903: 💤 Low valueNice coverage of metadata injection and filter behavior — small assertion tightening suggestion.
The metadata-injection assertions (lines 895-900) are exactly the kind of structural checks the happy-path tests are missing. One small thing: the negative branch at line 902 uses
!hasScript || len(scriptSection) == 0. WhenhasScriptis false,scriptSectionis the zero value — fine — but the intent reads more clearly as two separate assertions, e.g.assert.NotContains(t, components, "script"). Optional.🤖 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 `@internal/exec/stack_processor_process_stacks_test.go` around lines 882 - 903, The negative assertion for script components is ambiguous; instead of asserting !hasScript || len(scriptSection) == 0, replace it with a clearer check that the "script" key is absent from components (e.g., use assert.NotContains(t, components, "script")) or split into two explicit assertions: assert.False(t, hasScript) and if hasScript then assert.Len(t, scriptSection, 0). Update references around components, scriptSection, deployApp and the cfg.* symbols (cfg.ComponentTypeSectionName, cfg.VarsSectionName) accordingly so the intent is unambiguous.
350-732: ⚡ Quick winHappy-path assertions are weak — consider asserting structure, not just non-nil.
Nearly every
validateResultinTestProcessStackConfig_HappyPathisassert.NotNil(t, result). SinceProcessStackConfigalways returns a non-nil map whenerr == nil, these cases effectively only assertNoError. For the cases that exercise specific sections (e.g., terraform backend, providers, hooks, stack name override), a couple of targeted assertions on the returnedcomponents/namekeys would catch real regressions without much added effort.🤖 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 `@internal/exec/stack_processor_process_stacks_test.go` around lines 350 - 732, The test TestProcessStackConfig_HappyPath uses shallow assertions (validateResult closures calling assert.NotNil on result) which only check for non-nil maps; update each validateResult in that table (the closures passed in the test cases) to assert specific structure and values for the sections under test (e.g., for the "config with stack-level name override" case assert result["name"] == "custom-stack-name"; for terraform backend/provider/hook cases assert presence and expected keys under result["components"] or result["terraform"] such as backend.bucket, providers.aws.region, hooks.before_init; for env/vars/settings/auth/components cases assert the corresponding keys exist and have expected values) so TestProcessStackConfig_HappyPath fails on real regressions rather than only nil checks.examples/custom-components/components/script/deploy-app/deploy.sh (1)
2-5: 💤 Low valueSmall wording nit on the comment.
The comment says "uses the component vars via Go templates," but
${APP_NAME}/${VERSION}/${REPLICAS}in this script are plain shell variables, not Go template expressions. Since this file is illustrative-only, consider clarifying so readers don't expect Go-template substitution here.🤖 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 `@examples/custom-components/components/script/deploy-app/deploy.sh` around lines 2 - 5, Update the comment to avoid implying Go template syntax where shell variables are used: change the line that says "The custom command uses the component vars via Go templates" to clarify that this script is illustrative and uses plain shell variables (e.g., ${APP_NAME}, ${VERSION}, ${REPLICAS}) rather than Go-template expressions; ensure the new comment mentions that the real custom command may source or substitute these values via Go templates but this example simply demonstrates shell variable usage in deploy.sh.
🤖 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/cmd_utils.go`:
- Around line 1506-1550: Add performance tracking to processCustomComponentType
by inserting a defer perf.Track(nil, "cmd.processCustomComponentType")() as the
very first statement in the function (immediately after the signature) and leave
a blank line after that defer; ensure you reference the perf.Track helper and
the function name exactly "cmd.processCustomComponentType" so the call reads
perf.Track(nil, "cmd.processCustomComponentType")().
In `@examples/custom-components/README.md`:
- Line 11: The README.md contains fenced code blocks without a language hint
(MD040); update each plain triple-backtick block that shows directory or snippet
labels to include a language hint such as text so markdownlint stops flagging
them — specifically change the block containing the string
"examples/custom-components/" and the block containing "Component: deploy-app"
(and the other occurrence at the same section referenced as 40-40) to use
```text fenced blocks; ensure all similar untargeted triple-backtick blocks in
the file are updated the same way.
In `@internal/exec/stack_processor_process_stacks.go`:
- Around line 596-632: The three adjacent merges for vars/settings/env use
inconsistent write semantics (vars always overwritten, settings/env only written
when non-empty) and perform only shallow copies; make the behavior consistent
and perform deep merges: change the vars logic so
componentMap[cfg.VarsSectionName] is only set if the merged componentVars is
non-empty (same rule as SettingsSectionName and EnvSectionName), and replace the
manual range-based shallow merge for each of
componentVars/componentSettings/componentEnv with a deep merge using the
existing m.Merge helper (use m.Merge to merge global section into the component
section or vice-versa so nested maps are merged rather than replaced),
referencing the existing identifiers componentMap, cfg.VarsSectionName,
cfg.SettingsSectionName, cfg.EnvSectionName,
componentVars/componentSettings/componentEnv and m.Merge to locate and update
the code.
- Around line 584-587: The custom-component branch silently continues when
components cannot be asserted to map[string]any (componentsMap check), which
hides malformed entries; change that to return an explicit error consistent with
the built-in handlers (like ErrInvalidComponentsTerraform/Helmfile/Packer)
instead of continue so callers see the problem—either reuse an existing
ErrInvalidComponentsCustom (or introduce one) and return it with context naming
the offending component key, matching the inner non-map check behavior near the
later validation.
---
Nitpick comments:
In `@examples/custom-components/components/script/deploy-app/deploy.sh`:
- Around line 2-5: Update the comment to avoid implying Go template syntax where
shell variables are used: change the line that says "The custom command uses the
component vars via Go templates" to clarify that this script is illustrative and
uses plain shell variables (e.g., ${APP_NAME}, ${VERSION}, ${REPLICAS}) rather
than Go-template expressions; ensure the new comment mentions that the real
custom command may source or substitute these values via Go templates but this
example simply demonstrates shell variable usage in deploy.sh.
In `@internal/exec/stack_processor_process_stacks_test.go`:
- Around line 882-903: The negative assertion for script components is
ambiguous; instead of asserting !hasScript || len(scriptSection) == 0, replace
it with a clearer check that the "script" key is absent from components (e.g.,
use assert.NotContains(t, components, "script")) or split into two explicit
assertions: assert.False(t, hasScript) and if hasScript then assert.Len(t,
scriptSection, 0). Update references around components, scriptSection, deployApp
and the cfg.* symbols (cfg.ComponentTypeSectionName, cfg.VarsSectionName)
accordingly so the intent is unambiguous.
- Around line 350-732: The test TestProcessStackConfig_HappyPath uses shallow
assertions (validateResult closures calling assert.NotNil on result) which only
check for non-nil maps; update each validateResult in that table (the closures
passed in the test cases) to assert specific structure and values for the
sections under test (e.g., for the "config with stack-level name override" case
assert result["name"] == "custom-stack-name"; for terraform
backend/provider/hook cases assert presence and expected keys under
result["components"] or result["terraform"] such as backend.bucket,
providers.aws.region, hooks.before_init; for env/vars/settings/auth/components
cases assert the corresponding keys exist and have expected values) so
TestProcessStackConfig_HappyPath fails on real regressions rather than only nil
checks.
🪄 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: c89d5d34-5153-4ec8-bcc9-81b31b9a2fed
📒 Files selected for processing (26)
cmd/cmd_semantic_completion.gocmd/cmd_semantic_completion_test.gocmd/cmd_utils.gocmd/cmd_utils_test.goerrors/errors.goexamples/custom-components/README.mdexamples/custom-components/atmos.yamlexamples/custom-components/components/script/deploy-app/deploy.shexamples/custom-components/stacks/catalog/script/deploy-app.yamlexamples/custom-components/stacks/deploy/dev.yamlinternal/exec/describe_component.gointernal/exec/describe_component_test.gointernal/exec/stack_processor_process_stacks.gointernal/exec/stack_processor_process_stacks_test.gointernal/exec/vendor_utils.gopkg/component/custom/provider.gopkg/component/custom/provider_test.gopkg/component/registry.gopkg/component/registry_test.gopkg/schema/command.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/blog/2025-12-20-custom-component-types.mdxwebsite/docs/cli/configuration/commands.mdxwebsite/plugins/file-browser/index.jswebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
Ben (Benbentwo)
marked this pull request as draft
May 21, 2026 20:18
Ben (Benbentwo)
changed the base branch from
main
to
feature/dev-3755-custom-component-types
June 1, 2026 18:42
Contributor
Author
|
CodeRabbit (@coderabbitai) full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/hooks/hooks.go (1)
147-154: 💤 Low valueLoop var
eshadows theinternal/execimport alias.It works fine today, but
eis the package alias on Line 10, so any future edit inside this loop that reaches for theexecpackage would silently grab the string instead. A quick rename keeps things unambiguous.♻️ Optional rename
-func hookMatchesEvent(hook Hook, event HookEvent) bool { - for _, e := range hook.Events { - if NormalizeEvent(e) == event { - return true - } - } - return false -} +func hookMatchesEvent(hook Hook, event HookEvent) bool { + for _, candidate := range hook.Events { + if NormalizeEvent(candidate) == event { + return true + } + } + return false +}🤖 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 `@pkg/hooks/hooks.go` around lines 147 - 154, The loop variable `e` in function hookMatchesEvent shadows the package import alias `exec`; rename the loop variable to a more descriptive name (e.g., evt or eventStr) in hookMatchesEvent so it no longer collides with the import alias, update all uses inside that loop (including the call to NormalizeEvent) to the new name, and run `go vet`/tests to ensure no other references rely on the old short name.
🤖 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/cmd_utils.go`:
- Around line 670-684: The temp-file creation error currently uses fmt.Errorf
directly; update the CreateTemp error handling to wrap the underlying err with
the appropriate static error from errors/errors.go (use errors.Join or
fmt.Errorf with the static error's %w as per project convention) before passing
it to errUtils.CheckErrorPrintAndExit; modify the block around os.CreateTemp,
the variable outputsFilePath, and the err passed into
errUtils.CheckErrorPrintAndExit so the logged/returned error is the static error
combined with the original err (ensure the static error symbol from
errors/errors.go is referenced).
In `@docs/prd/hooks-for-custom-components.md`:
- Around line 349-368: The Implementation plan wrongly instructs adding both
BeforeApply and AfterApply constants while the code and design scope exclude
before-apply; update the plan to remove any reference to BeforeApply and state
only AfterApply is added (matching pkg/hooks/event.go where AfterApply is
defined), and ensure other steps (store_cmd.go, cmd_utils.go, docs) do not
reference or implement BeforeApply.
In `@pkg/hooks/hooks.go`:
- Around line 61-64: The yaml.Unmarshal error path returns nil for *Hooks which
is inconsistent with other error branches and can cause a panic when callers
call the value-receiver method HasHooks; change the error return to return
&Hooks{} along with the wrapped error. Specifically, in the block where
yaml.Unmarshal(yamlData, &items) fails, replace the nil *Hooks return with
&Hooks{} so functions/methods like Hooks.HasHooks and other callers always
receive a non-nil *Hooks even on error.
---
Nitpick comments:
In `@pkg/hooks/hooks.go`:
- Around line 147-154: The loop variable `e` in function hookMatchesEvent
shadows the package import alias `exec`; rename the loop variable to a more
descriptive name (e.g., evt or eventStr) in hookMatchesEvent so it no longer
collides with the import alias, update all uses inside that loop (including the
call to NormalizeEvent) to the new name, and run `go vet`/tests to ensure no
other references rely on the old short name.
🪄 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: 471f3818-a089-45d0-afc6-d7379e278343
📒 Files selected for processing (21)
cmd/cmd_utils.gocmd/cmd_utils_fires_test.gocmd/internal/runhooks.gocmd/terraform/apply.gocmd/terraform/deploy.gocmd/terraform/utils.godocs/prd/hooks-for-custom-components.mderrors/errors.gopkg/hooks/event.gopkg/hooks/event_test.gopkg/hooks/hooks.gopkg/hooks/hooks_test.gopkg/hooks/outputs_file.gopkg/hooks/outputs_file_test.gopkg/hooks/store_cmd.gopkg/hooks/store_cmd_custom_test.gopkg/hooks/store_cmd_nil_handling_test.gopkg/hooks/store_cmd_test.gopkg/schema/command.gopkg/schema/schema.gopkg/store/aws_ssm_param_store.go
Contributor
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Action performedReview finished.
|
Ben (Benbentwo)
marked this pull request as ready for review
June 3, 2026 21:30
Ben (Benbentwo)
added a commit
to Benbentwo/atmos
that referenced
this pull request
Jun 3, 2026
- Status: Draft -> Implemented (PR cloudposse#2469, stacked on/pending merge into cloudposse#1904) - Section 3: describe the two typed getters actually built (TerraformOutputGetter + CustomOutputGetter dispatched via isTerraformComponent) instead of the original single-getter sketch; note the outputs file parses JSON or KEY=VALUE. - Section 4: replace the stale generic AfterApply firing prose with the before/after.<type>.<subcommand> fireComponentHook flow that shipped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Ben (@Benbentwo)? 🙏 |
Custom component types fire `<phase>.<type>.<subcommand>` lifecycle hooks (e.g. before.agent.greeting / after.agent.greeting) and publish outputs via $ATMOS_OUTPUTS so a `store` hook can persist results — closing the run → outputs → next-run loop Terraform already has. - pkg/hooks: ComponentEvent(phase,type,subcommand) helper + PhaseBefore/After; HooksFromComponent builds hooks from an already-resolved component map (avoids re-describing custom types the built-in describe path can't see); store_cmd dispatches output resolution on component type (terraform getter vs ATMOS_OUTPUTS file via defaultCustomOutputter); outputs_file parses JSON or KEY=VALUE. - cmd: cmd/internal/runhooks.go shared RunHooks with optional preResolvedComponent; custom command path fires before/after derived events. - internal/exec: custom component types deep-merge global vars/settings/env via m.Merge (matching built-in inheritance) and error on malformed config. - errors: ErrReadOutputsFile, ErrCreateOutputsFile, ErrCustomOutputMissing. - examples/custom-components: hello-world with a post-run store hook (Redis). - docs: PRD, website hooks docs, changelog blog, roadmap milestone. - tests: event naming, outputs-file parsing, store dispatch, SSM WithDecryption. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Ben (Benbentwo)
force-pushed
the
benbentwo/dev-3755-custom-component-types
branch
from
June 4, 2026 18:48
4c01670 to
e0c2622
Compare
Contributor
|
💥 This pull request now has conflicts. Could you fix it Ben (@Benbentwo)? 🙏 |
Andriy Knysh (aknysh)
deleted the branch
cloudposse:feature/dev-3755-custom-component-types
June 4, 2026 23:59
Contributor
Ben (Benbentwo)
added a commit
to Benbentwo/atmos
that referenced
this pull request
Jun 8, 2026
- outputs_file: strip exactly one pair of surrounding quotes (single or double) instead of greedily trimming all quote chars, so an inner quote survives (e.g. "'hello'" -> 'hello'). Add a nested-quote test. - outputs_file: add a CRLF test documenting that TrimSpace already strips trailing \r from Windows line endings (the flagged concern was already handled — no code change needed). - docs/roadmap: correct stale PR references from cloudposse#2469 to cloudposse#2584; update the PRD status (custom component types cloudposse#1904 has merged to main). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Ben (Benbentwo)
added a commit
to Benbentwo/atmos
that referenced
this pull request
Jun 9, 2026
- outputs_file: strip exactly one pair of surrounding quotes (single or double) instead of greedily trimming all quote chars, so an inner quote survives (e.g. "'hello'" -> 'hello'). Add a nested-quote test. - outputs_file: add a CRLF test documenting that TrimSpace already strips trailing \r from Windows line endings (the flagged concern was already handled — no code change needed). - docs/roadmap: correct stale PR references from cloudposse#2469 to cloudposse#2584; update the PRD status (custom component types cloudposse#1904 has merged to main). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Ben (Benbentwo)
added a commit
to Benbentwo/atmos
that referenced
this pull request
Jun 10, 2026
- outputs_file: strip exactly one pair of surrounding quotes (single or double) instead of greedily trimming all quote chars, so an inner quote survives (e.g. "'hello'" -> 'hello'). Add a nested-quote test. - outputs_file: add a CRLF test documenting that TrimSpace already strips trailing \r from Windows line endings (the flagged concern was already handled — no code change needed). - docs/roadmap: correct stale PR references from cloudposse#2469 to cloudposse#2584; update the PRD status (custom component types cloudposse#1904 has merged to main). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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
3 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.
Summary
What Changed
pkg/component/custom)Why This Matters
This feature enables custom commands to provide superior developer experience through:
References
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
firesfieldafter-applyevent with legacy alias supportImprovements
Docs & Tests