restore hooks functionality #1010
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
mcalhoun
added
the
no-release
Graphite App
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
a3110ed to
bf1232e
Compare
mcalhoun
force-pushed
the
aws-ssm-param-store-get-key
branch
from
2ad5ece to
5a28ac1
Compare
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
748b99d to
1e175cb
Compare
mcalhoun
force-pushed
the
aws-ssm-param-store-get-key
branch
2 times, most recently
from
8563f33 to
13ffe7f
Compare
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
1e175cb to
1501ce7
Compare
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
1e6a304 to
ca0306b
Compare
mcalhoun
force-pushed
the
aws-ssm-param-store-get-key
branch
from
f94f4cd to
b4816f7
Compare
mcalhoun
changed the base branch from
aws-ssm-param-store-get-key
to
graphite-base/1010
mcalhoun
force-pushed
the
graphite-base/1010
branch
from
b4816f7 to
2a38feb
Compare
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
ca0306b to
da33ee9
Compare
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
da33ee9 to
7211dae
Compare
📝 WalkthroughWalkthroughThis pull request refactors hook handling within the Terraform commands. A new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant TC as Terraform Command
participant RH as runHooks Function
participant H as Hooks Manager
participant SC as StoreCommand
CLI->>TC: Execute Terraform command
TC->>RH: Invoke runHooks(event, cmd, args)
RH->>H: Fetch hooks configuration (GetHooks)
H-->>RH: Return hooks configuration
RH->>H: Execute hooks (RunAll)
alt Hook requires store command
H->>SC: Instantiate StoreCommand and call RunE
SC-->>H: Return stored output result
end
H-->>RH: Completed hook execution
RH-->>TC: Return result
TC-->>CLI: Final command output
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
pkg/hooks/hooks.go (3)
20-23: Consider defensive checks for the 'hooks' key before casting.If “hooks” is missing,
sections["hooks"].(map[string]any)could panic. Returningnilinstead of&Hooks{}on errors is also more idiomatic.if val, ok := sections["hooks"].(map[string]any); ok { hooksSection := val ... } else { - return &Hooks{}, fmt.Errorf("no hooks section found") + return nil, fmt.Errorf("no hooks section found") }Also applies to: 26-37, 39-46
48-66: Handle unknown commands or provide feedback.Currently, only “store” is handled. A default case or a way to notify users about unrecognized commands might be helpful.
switch hook.Command { case "store": ... default: + log.Warn("Unsupported hook command", "command", hook.Command) }
68-69: Implement or remove this placeholder.Returning an empty
Hookswithout converting could lead to confusion.Would you like me to propose an implementation or remove it if unused?
pkg/hooks/hook.go (1)
3-14: LGTM! Consider enhancing documentation.The
Hookstruct is well-designed with clear YAML tags and good separation of concerns. The documentation is clear, but could be enhanced with examples.Consider adding usage examples in the documentation:
// Hook is the structure for a hook and is using in the stack config to define // a command that should be run when a specific event occurs. +// +// Example: +// hooks: +// - events: ["after.terraform.apply"] +// command: "store" +// name: "testredis" +// outputs: +// random_string: ".random_string" type Hook struct {main_test.go (1)
33-42: Consider enhancing test assertions.The test successfully validates the integration flow but could benefit from explicit assertions on the Redis state.
Consider adding assertions to verify the Redis state after each deployment:
os.Args = []string{"atmos", "terraform", "deploy", "random1", "-s", "test"} main() +// Verify Redis state after random1 deployment +if got := s.Get("test/random1/random_string"); got == "" { + t.Error("expected Redis to contain output from random1") +} os.Args = []string{"atmos", "terraform", "deploy", "random2", "-s", "test"} main() +// Verify Redis state after random2 deployment +if got := s.Get("test/random2/random_string"); got == "" { + t.Error("expected Redis to contain output from random2") +}cmd/terraform.go (1)
45-60: LGTM! Consider adding debug logging.The
runHooksfunction is well-structured with proper error handling. Consider adding debug logging for better observability.func runHooks(event h.HookEvent, cmd *cobra.Command, args []string) error { + log.Debug("running hooks", "event", event, "command", cmd.Name()) info := getConfigAndStacksInfo("terraform", cmd, append([]string{cmd.Name()}, args...)) // Initialize the CLI config atmosConfig, err := cfg.InitCliConfig(info, true) if err != nil { + log.Debug("failed to initialize CLI config", "error", err) return fmt.Errorf("error initializing CLI config: %w", err) }pkg/hooks/store_cmd.go (2)
57-61: Optimize string comparison.The string index check could be replaced with a more idiomatic Go approach.
-if strings.Index(value, ".") == 0 { +if strings.HasPrefix(value, ".") { outputValue = e.GetTerraformOutput(c.atmosConfig, c.info.Stack, c.info.ComponentFromArg, outputKey, true) } else { outputValue = value }
65-77: LGTM! Consider adding validation.The store output function is well-implemented with proper error handling and logging.
Consider adding validation for the output value:
func (c *StoreCommand) storeOutput(hook *Hook, key string, outputKey string, outputValue any) error { + if outputValue == nil { + return fmt.Errorf("output value for key %q cannot be nil", key) + } + log.Debug("checking if the store exists", "store", hook.Name) store := c.atmosConfig.Stores[hook.Name]testdata/fixtures/hooks-test/components/terraform/random/main.tf (1)
1-20: Well-structured test configuration!The configuration properly defines variables, uses triggers for updates, and exposes the required output for hook testing.
Consider adding validation for the stage variable to ensure it matches expected values.
variable "stage" { description = "Stage where it will be deployed" type = string + validation { + condition = contains(["test", "dev", "prod"], var.stage) + error_message = "Stage must be one of: test, dev, prod" + } }testdata/fixtures/hooks-test/atmos.yaml (1)
1-25: Solid configuration for hook testing!The configuration properly sets up all required components: Redis store, Terraform settings, stack configuration, and logging.
Consider adding Redis connection settings for production use.
stores: testredis: type: redis + host: ${REDIS_HOST:-localhost} + port: ${REDIS_PORT:-6379} + db: ${REDIS_DB:-0} + username: ${REDIS_USERNAME:-""} + password: ${REDIS_PASSWORD:-""}testdata/fixtures/hooks-test/stacks/stack.yaml (1)
1-23: Excellent hook and store integration setup!The configuration demonstrates proper hook usage and store value sharing between components.
Consider adding a description to the hook for better maintainability.
hooks: store-outputs: + description: "Stores the random value for use by random2 component" events: - after-terraform-apply
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/terraform.go(2 hunks)cmd/terraform_commands.go(4 hunks)go.mod(2 hunks)main_test.go(1 hunks)pkg/hooks/cmd.go(0 hunks)pkg/hooks/command.go(1 hunks)pkg/hooks/event.go(1 hunks)pkg/hooks/hook.go(1 hunks)pkg/hooks/hooks.go(1 hunks)pkg/hooks/store.go(0 hunks)pkg/hooks/store_cmd.go(1 hunks)testdata/fixtures/hooks-test/atmos.yaml(1 hunks)testdata/fixtures/hooks-test/components/terraform/random/main.tf(1 hunks)testdata/fixtures/hooks-test/stacks/stack.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/hooks/store.go
- pkg/hooks/cmd.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
main_test.go
25-25: Error return value of os.Chdir is not checked
(errcheck)
27-27: Error return value of os.Chdir is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/hooks/hooks.go (2)
6-10: Imports look appropriate.They are used consistently in the file. No issues here.
14-18: Struct composition is clear.These fields provide a good container for config, stack info, and hook items.
pkg/hooks/command.go (1)
1-9: Interface design looks good.This is straightforward for hooking different commands.
pkg/hooks/event.go (1)
1-10: Event constants enable clean comparisons.Nicely keeps Terraform event strings in one place.
cmd/terraform_commands.go (3)
6-7: LGTM! Hook execution properly integrated into apply command.The implementation correctly logs hook execution and triggers the AfterTerraformApply event.
Also applies to: 32-35
164-172: Documentation improvements enhance clarity!The updated documentation clearly explains the region variable handling and command arguments.
66-69: LGTM! Consistent hook handling in deploy command.The deploy command correctly uses the same hook event as apply, maintaining consistency.
go.mod (2)
9-9: Miniredis Dependency AdditionThe inclusion of
github.com/alicebob/miniredis/v2 v2.34.0directly supports your integration tests for the new Redis-based store functionality. Please confirm that this version meshes well with your test scenarios and overall environment.
85-85: Gopher-JSON Indirect Dependency CheckThe dependency
github.com/alicebob/gopher-json v0.0.0-20230218143504-906a9b012302is added as an indirect requirement. Please verify that it is indeed required by one of your direct dependencies and that the pinned version meets your compatibility expectations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
main_test.go (1)
25-25:⚠️ Potential issueAdd error handling for directory restoration.
The deferred directory restoration should check for errors.
-defer os.Chdir(origDir) +defer func() { + if err := os.Chdir(origDir); err != nil { + // In deferred functions, we can't return an error or fail the test directly + // Best practice is to log the error or panic + panic(fmt.Sprintf("failed to restore original directory: %v", err)) + } +}()🧰 Tools
🪛 golangci-lint (1.62.2)
25-25: Error return value of
os.Chdiris not checked(errcheck)
🧹 Nitpick comments (1)
main_test.go (1)
35-44: Consider enhancing test robustness.The test execution could be improved in several ways:
- Add assertions to verify the Redis store contains expected values after each main() call
- Consider capturing and validating main()'s output
- Add error handling for main() execution
Here's an example enhancement:
os.Args = []string{"atmos", "terraform", "deploy", "random1", "-s", "test"} -main() +// Capture stdout for verification +oldStdout := os.Stdout +r, w, _ := os.Pipe() +os.Stdout = w + +main() + +w.Close() +os.Stdout = oldStdout + +// Verify Redis store +val, err := s.Get("your_expected_key") +if err != nil { + t.Fatalf("failed to get value from Redis: %v", err) +} +if val != "expected_value" { + t.Errorf("expected value %q, got %q", "expected_value", val) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
main_test.go
25-25: Error return value of os.Chdir is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
main_test.go (2)
12-19: LGTM! Clean Redis test setup.The Redis mock setup is well-structured with proper cleanup using defer.
31-34: LGTM! Clean arguments management.The original arguments are properly captured and restored.
mcalhoun
force-pushed
the
restore-hooks-functionality
branch
from
9aa02a6 to
d14853c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/terraform.go (1)
47-67: Enhance logging for better observabilityThe function has good error handling, but could benefit from additional debug logs to help troubleshoot hook execution issues.
func runHooks(event h.HookEvent, cmd *cobra.Command, args []string) error { + l.Debug("starting hook execution", "event", event, "command", cmd.Name()) info := getConfigAndStacksInfo("terraform", cmd, append([]string{cmd.Name()}, args...))cmd/terraform_commands.go (1)
31-33: Consider adding validation for argsThe PostRunE hooks could benefit from validating that required arguments are present before executing the hooks.
PostRunE: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return fmt.Errorf("component argument is required") + } return runHooks(h.AfterTerraformApply, cmd, args) },Also applies to: 64-66
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/terraform.go(2 hunks)cmd/terraform_commands.go(4 hunks)pkg/hooks/hooks.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/hooks/hooks.go (1)
14-18: Well-structured Hooks type!The Hooks struct provides a clean encapsulation of configuration, info, and hook items. This design promotes better organization and maintainability.
cmd/terraform_commands.go (1)
160-169: Excellent documentation update!The import command documentation is clear, comprehensive, and well-structured. It effectively explains the region handling and required arguments.
| func GetHooks(atmosConfig *schema.AtmosConfiguration, info *schema.ConfigAndStacksInfo) (*Hooks, error) { | ||
| sections, err := e.ExecuteDescribeComponent(info.ComponentFromArg, info.Stack, true, true, []string{}) | ||
| if err != nil { | ||
| return &Hooks{}, fmt.Errorf("failed to execute describe component: %w", err) | ||
| } | ||
|
|
||
| func (h Hooks) ConvertToHooks(input map[string]any) (Hooks, error) { | ||
| yamlData, err := yaml.Marshal(input) | ||
| hooksSection := sections["hooks"].(map[string]any) | ||
|
|
||
| yamlData, err := yaml.Marshal(hooksSection) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal input: %w", err) | ||
| return &Hooks{}, fmt.Errorf("failed to marshal hooksSection: %w", err) | ||
| } | ||
|
|
||
| var hooks Hooks | ||
| err = yaml.Unmarshal(yamlData, &hooks) | ||
| var items map[string]Hook | ||
| err = yaml.Unmarshal(yamlData, &items) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal to Hooks: %w", err) | ||
| } | ||
|
|
||
| return hooks, nil | ||
| hooks := Hooks{ | ||
| config: atmosConfig, | ||
| info: info, | ||
| items: items, | ||
| } | ||
|
|
||
| return &hooks, nil | ||
| } |
There was a problem hiding this comment.
Add nil check for hooksSection
The type assertion on line 30 could panic if the "hooks" section is nil. Add a safety check.
- hooksSection := sections["hooks"].(map[string]any)
+ hooksSection, ok := sections["hooks"].(map[string]any)
+ if !ok {
+ return &Hooks{}, fmt.Errorf("hooks section is missing or invalid")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetHooks(atmosConfig *schema.AtmosConfiguration, info *schema.ConfigAndStacksInfo) (*Hooks, error) { | |
| sections, err := e.ExecuteDescribeComponent(info.ComponentFromArg, info.Stack, true, true, []string{}) | |
| if err != nil { | |
| return &Hooks{}, fmt.Errorf("failed to execute describe component: %w", err) | |
| } | |
| func (h Hooks) ConvertToHooks(input map[string]any) (Hooks, error) { | |
| yamlData, err := yaml.Marshal(input) | |
| hooksSection := sections["hooks"].(map[string]any) | |
| yamlData, err := yaml.Marshal(hooksSection) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to marshal input: %w", err) | |
| return &Hooks{}, fmt.Errorf("failed to marshal hooksSection: %w", err) | |
| } | |
| var hooks Hooks | |
| err = yaml.Unmarshal(yamlData, &hooks) | |
| var items map[string]Hook | |
| err = yaml.Unmarshal(yamlData, &items) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal to Hooks: %w", err) | |
| } | |
| return hooks, nil | |
| hooks := Hooks{ | |
| config: atmosConfig, | |
| info: info, | |
| items: items, | |
| } | |
| return &hooks, nil | |
| } | |
| func GetHooks(atmosConfig *schema.AtmosConfiguration, info *schema.ConfigAndStacksInfo) (*Hooks, error) { | |
| sections, err := e.ExecuteDescribeComponent(info.ComponentFromArg, info.Stack, true, true, []string{}) | |
| if err != nil { | |
| return &Hooks{}, fmt.Errorf("failed to execute describe component: %w", err) | |
| } | |
| // Safely assert hooksSection to avoid nil pointer dereference | |
| hooksSection, ok := sections["hooks"].(map[string]any) | |
| if !ok { | |
| return &Hooks{}, fmt.Errorf("hooks section is missing or invalid") | |
| } | |
| yamlData, err := yaml.Marshal(hooksSection) | |
| if err != nil { | |
| return &Hooks{}, fmt.Errorf("failed to marshal hooksSection: %w", err) | |
| } | |
| var items map[string]Hook | |
| err = yaml.Unmarshal(yamlData, &items) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal to Hooks: %w", err) | |
| } | |
| hooks := Hooks{ | |
| config: atmosConfig, | |
| info: info, | |
| items: items, | |
| } | |
| return &hooks, nil | |
| } |
|
These changes were released in v1.159.0. |
* restore hooks functionality * [autofix.ci] apply automated fixes * Update main_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
what
after.terraform.applyeventswhy
references
Summary by CodeRabbit
New Features
StoreCommandstructure for handling outputs within hooks.Bug Fixes
Documentation