fix store bug and allow for hooks at multiple levels of config #1112
Conversation
📝 WalkthroughWalkthroughThe change set enhances hooks handling in configuration processing and expands test coverage across several storage components. The modifications introduce new error constants and variables to validate and merge hook configurations in the code, update file permission modes and method signatures in testing helpers, and enhance the processing of JSON values in AWS SSM operations. Additionally, YAML configurations are updated to shift hook definitions from component-specific to a centralized global section. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as "Stack Config"
participant Processor as "ProcessStackConfig()"
participant Validator as "Hooks Validator/Merger"
Config->>Processor: Provide configuration data
Processor->>Validator: Check for "hooks" section
alt Valid hooks section found
Validator-->>Processor: Return valid hooks data
Processor->>Validator: Extract and validate "terraformHooks"
alt Valid terraform hooks
Validator-->>Processor: Confirm valid terraform hooks
Processor->>Processor: Merge global hooks, terraform hooks, and component hooks
else Invalid terraform hooks
Validator-->>Processor: Return ErrInvalidTerraformHooksSection
end
else Invalid hooks section
Validator-->>Processor: Return ErrInvalidHooksSection
end
Processor-->>Config: Return final processed configuration
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.1) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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. 🪧 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1112 +/- ##
==========================================
+ Coverage 17.17% 17.29% +0.11%
==========================================
Files 169 169
Lines 18725 18747 +22
==========================================
+ Hits 3216 3242 +26
+ Misses 14924 14908 -16
- Partials 585 597 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (4)
pkg/store/aws_ssm_param_store.go (1)
149-153: Add parameter name in error message for better troubleshootingThe error message when JSON unmarshaling fails doesn't include the parameter name, making troubleshooting more difficult.
- return nil, fmt.Errorf("failed to unmarshal parameter value: %w", err) + return nil, fmt.Errorf("failed to unmarshal parameter value for '%s': %w", paramName, err)pkg/store/aws_ssm_param_store_test.go (1)
116-120: Use wrapped static errors instead of dynamic errorsThe static analysis tool identified an issue with using dynamic errors. Go best practices recommend using wrapped static errors.
- m.On("PutParameter", mock.Anything, mock.Anything). - Return(nil, errors.New("invalid value type")) + // Define a static error at package level + m.On("PutParameter", mock.Anything, mock.Anything). + Return(nil, fmt.Errorf("mock error: %w", errInvalidValueType))Add this at the package level:
var errInvalidValueType = errors.New("invalid value type")🧰 Tools
🪛 GitHub Check: golangci
[failure] 118-118: [golangci] pkg/store/aws_ssm_param_store_test.go#L118
do not define dynamic errors, use wrapped static errors instead: "errors.New("invalid value type")" (err113)internal/exec/stack_processor_utils.go (2)
575-580: Use wrapped static errors instead of dynamic errorsThe error creation should use a wrapped static error to improve error handling patterns.
- return nil, fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName) + const errInvalidHooksSection = "invalid hooks section in the file" + return nil, fmt.Errorf("%s: %s", errInvalidHooksSection, stackName)🧰 Tools
🪛 GitHub Check: golangci
[failure] 578-578: [golangci] internal/exec/stack_processor_utils.go#L578
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName)" (err113)
632-637: Use wrapped static errors for terraform hooks validationSimilar to the previous error handling pattern, use wrapped static errors here too.
- return nil, fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName) + const errInvalidTerraformHooks = "invalid terraform.hooks section in the file" + return nil, fmt.Errorf("%s: %s", errInvalidTerraformHooks, stackName)🧰 Tools
🪛 GitHub Check: golangci
[failure] 635-635: [golangci] internal/exec/stack_processor_utils.go#L635
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/exec/stack_processor_utils.go(6 hunks)pkg/store/artifactory_store_test.go(3 hunks)pkg/store/aws_ssm_param_store.go(3 hunks)pkg/store/aws_ssm_param_store_test.go(5 hunks)pkg/store/redis_store_test.go(1 hunks)tests/fixtures/scenarios/hooks-test/stacks/stack.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/store/aws_ssm_param_store_test.go
[failure] 118-118: [golangci] pkg/store/aws_ssm_param_store_test.go#L118
do not define dynamic errors, use wrapped static errors instead: "errors.New("invalid value type")" (err113)
pkg/store/artifactory_store_test.go
[failure] 50-50: [golangci] pkg/store/artifactory_store_test.go#L50
G306: Expect WriteFile permissions to be 0600 or less (gosec)
internal/exec/stack_processor_utils.go
[failure] 578-578: [golangci] internal/exec/stack_processor_utils.go#L578
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName)" (err113)
[failure] 635-635: [golangci] internal/exec/stack_processor_utils.go#L635
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
🪛 golangci-lint (1.62.2)
pkg/store/artifactory_store_test.go
[medium] 50-50: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
⏰ 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 (16)
tests/fixtures/scenarios/hooks-test/stacks/stack.yaml (3)
1-9: Good implementation of global hooks sectionThe move to define hooks at the global level aligns perfectly with the PR objective of allowing hooks at the root configuration. This approach enables reuse across components and simplifies configuration management.
27-28:❓ Verification inconclusive
Verify store reference format works with JSON serialization
Line 28 uses the
!storedirective to reference a value stored by the hook from component1. Confirm this syntax is compatible with the new JSON serialization implementation in the underlying store.
🏁 Script executed:
#!/bin/bash # Check for similar !store references in the codebase and verify compatibility rg -A 2 "!store" --type yaml # Check how these references are processed rg -A 10 "!store.*process|process.*!store" --type goLength of output: 193
Action: Confirm JSON Serialization Handling for
!storeDirectiveAfter reviewing the repository, our search showed that the
!storereference exists only intests/fixtures/scenarios/hooks-test/stacks/stack.yaml(lines 27–28). There is no additional Go code specifically processing or modifying these references during JSON serialization. However, since our automated grep commands didn’t reveal any explicit JSON serialization logic tied to the!storesyntax, it's important to manually verify that the new JSON serialization implementation correctly preserves and interprets these directives.• Location to verify:
tests/fixtures/scenarios/hooks-test/stacks/stack.yaml(lines 27–28)
• Next step: Manually confirm through integration tests or direct inspection that the JSON output generated from YAML files retains the intended!storedirective behavior.
15-18:❓ Verification inconclusive
Confirm component-level hook references work correctly
The component-level hooks section now only contains outputs configuration rather than the full hook definition. Make sure that the hook framework correctly associates this component's outputs with the globally defined hook.
🏁 Script executed:
#!/bin/bash # Check if hook merging logic is correctly implemented in the codebase # Looking for code that merges global hooks with component-specific hooks rg -A 10 -B 5 "mergeHooks|merge.*hooks|hooks.*merge" --type goLength of output: 64
I'll run an additional check with a modified pattern to see if there's explicit evidence of hook merging logic in the Go codebase.
It appears that our previous search (and this additional one) didn’t return any definitive evidence of hook merging logic. Given these inconclusive results, please manually verify that the hook framework correctly links the component’s outputs (as specified in the file) to the globally defined hooks. Specifically, confirm in your tests or runtime logs that:
- In
tests/fixtures/scenarios/hooks-test/stacks/stack.yaml(Lines 15-18), thestore-outputshook’s outputs (e.g.random_id: .random) are properly merged or associated with the global hook definitions.- The expected merging or association logic is actually executed within your hook framework.
Make sure to run integration tests or add logging to validate that the output of this component-level configuration is correctly handled.
🏁 Script executed:
#!/bin/bash # Further checking for hook merging implementations using alternative patterns rg -A 10 -B 5 "MergeHooks|AssociateHooks|CombineHooks" --type goLength of output: 66
Action Required: Manually Verify Component-Level Hook Merging
The automated searches for merging logic in the Go code didn’t reveal explicit evidence of hook association for the component-level configuration. Please ensure that:
- The component-level hook defined in
tests/fixtures/scenarios/hooks-test/stacks/stack.yaml(lines 15–18) is correctly associated with the global hook definitions.- Integration tests or logging confirm that the outputs (e.g.,
random_id: .random) are properly merged with the global hook framework.Since our script searches returned no conclusive results, we recommend a thorough manual review to verify that the hook framework handles this component-level configuration as expected.
pkg/store/aws_ssm_param_store.go (1)
93-98: Good implementation of JSON serialization for valuesThe conversion to JSON format solves the issue with non-string values in SSM. This change ensures consistent serialization across all value types.
pkg/store/redis_store_test.go (2)
36-66: Effective test coverage with diverse data structuresThe table-driven tests with various data structures (maps, slices, nested objects) provide comprehensive coverage for JSON serialization/deserialization. This approach ensures the store can handle complex data types correctly.
98-101: Strong validation approach using JSONEqUsing JSONEq for comparison is the right approach as it verifies semantic equality of data structures regardless of formatting or field order differences in the JSON representation.
pkg/store/aws_ssm_param_store_test.go (2)
67-97: Good test coverage for JSON serializationThe new test cases for slices and maps validate that the SSM store correctly handles complex data types through JSON serialization.
237-272: Comprehensive test cases for complex data structuresThe additional test cases for retrieving slices and maps ensure the Get method correctly handles unmarshaling of different data structures. This provides coverage for all the common use cases.
pkg/store/artifactory_store_test.go (4)
44-44: Approved: Updated to modern octal notationThis change from decimal (0755) to modern octal (0o755) notation improves code clarity and readability.
57-59: Signature updated to match updated service interfaceThe mock client implementation has been properly updated to match changes in the Artifactory client interface, which now accepts options parameter.
148-168: Expanded test coverage for complex data typesGood addition of test cases for complex data structures (slices, maps, nested maps). This ensures the component correctly handles JSON serialization/deserialization of non-string values.
180-256: Comprehensive test coverage for SetKey methodStrong test implementation that validates the ArtifactoryStore.Set method with various data types and path configurations. The test ensures proper mocking of the UploadFiles method and validates the expected parameters are correctly passed.
The test coverage aligns perfectly with the PR objective of fixing store marshalling of non-string values to JSON format.
internal/exec/stack_processor_utils.go (4)
544-544: Added support for global hooks configurationThis addition enables hooks to be defined at the root level of config, as mentioned in the PR objectives.
644-647: Proper merge of global and terraform hooksGood implementation of merging hooks from both global and terraform-specific sources.
1111-1112: Updated component hooks merging to include global hooksThis change properly integrates the global hooks into the component's final hook configuration. It ensures hooks can be specified at multiple levels and are correctly merged.
1661-1662: Formatting and style improvementTrailing commas added to improve readability and make future parameter additions cleaner.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/store/aws_ssm_param_store_test.go (1)
116-119: Test case update looks good but may need more precision.The test correctly expects an error for invalid value types, though the expected error message doesn't match what would be returned by the JSON encoding failure. Consider making it more specific if needed.
internal/exec/stack_processor_utils.go (1)
637-642: Need to standardize error handling here too.This section uses
fmt.Errorfinstead of theerrors.Wrapfpattern used elsewhere. For consistency, consider using the same approach as with other sections.- return nil, fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName) + return nil, errors.Wrapf(ErrInvalidHooksSection, "terraform.hooks section in %s", stackName)🧰 Tools
🪛 GitHub Check: golangci
[failure] 640-640: [golangci] internal/exec/stack_processor_utils.go#L640
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
📜 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 (3)
go.mod(8 hunks)internal/exec/stack_processor_utils.go(7 hunks)pkg/store/aws_ssm_param_store_test.go(7 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
internal/exec/stack_processor_utils.go
[failure] 597-597: [golangci] internal/exec/stack_processor_utils.go#L597
add-constant: string literal "%s" appears, at least, 4 times, create a named constant for it (revive)
[failure] 640-640: [golangci] internal/exec/stack_processor_utils.go#L640
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (12)
pkg/store/aws_ssm_param_store_test.go (3)
59-63: Good update to use JSON serialization for parameter values.The changes properly marshal string values to JSON format which provides consistency for all value types.
67-97: Great addition of test cases for complex data types.Adding tests for slices and maps ensures the AWS SSM store properly handles non-primitive types through JSON serialization.
237-272: Well-structured test cases for retrieving complex types.These new tests properly validate that values stored as JSON strings are correctly unmarshaled back into their original data types. The test coverage is thorough.
internal/exec/stack_processor_utils.go (5)
29-32: Good standardization of error variables.Creating standard error variables improves consistency and will make error handling more maintainable.
576-597: Improved error handling with proper wrapping.Using
errors.Wrapf()with the standardized error variables enhances error reporting and provides better context.🧰 Tools
🪛 GitHub Check: golangci
[failure] 597-597: [golangci] internal/exec/stack_processor_utils.go#L597
add-constant: string literal "%s" appears, at least, 4 times, create a named constant for it (revive)
580-584: Good validation for the global hooks section.The added validation ensures the hooks section is properly formatted before processing it.
649-652: Good implementation of hooks merging.The added code correctly handles merging global hooks with terraform-specific hooks.
1113-1124: Updated hooks merging logic looks correct.The
finalComponentHooksnow includesglobalAndTerraformHooksas the first item in the merge list, ensuring hooks are properly inherited from all levels.go.mod (4)
69-76: Significant Google Cloud dependency updates.The addition of Google Cloud authentication and monitoring packages suggests new functionality that may need thorough testing.
Make sure to test any code that relies on these dependencies, especially authentication flows.
79-81: New OpenTelemetry dependencies added.These additions suggest new monitoring or tracing capabilities. Ensure that any code using these has appropriate tests.
127-129: Protocol buffer and RPC framework dependencies added.New dependencies for service frameworks and protocol validation have been added. This may indicate new communication capabilities being introduced.
Also applies to: 157-158
3-3:✅ Verification successful
Verify Go version compatibility.
The file shows Go version 1.23.5 which doesn't appear to be a valid released Go version (current is around 1.21-1.22 as of early 2024).
🌐 Web query:
What is the latest Go version?💡 Result:
The latest stable release of the Go programming language is version 1.24.0, released on February 11, 2025[1][3][4]. This release includes several new features and improvements:
Key updates in Go 1.24
- New
go toolcommand for enhanced development workflows[1]omitzerostruct field option to simplify JSON serialization[1]- Iterator functions in the
stringspackage[1]- Experimental
testing/synctestpackage for concurrency testing[1]- Full support for generic type aliases (removing the need for
GOEXPERIMENT=aliastypeparams)[5]Support status
- Go 1.24 is actively supported until two subsequent major releases (following Go's standard support policy)[3][4]
- Previous versions:
- Go 1.23 (supported until Go 1.25 release)
- Go 1.22 (support ended February 11, 2025)[3]
You can install the latest version from the official Go downloads page[6] or upgrade using package managers like Homebrew (
brew upgrade go)[1]. Developers using Go 1.21 or later can automatically receive toolchain updates through thegocommand's new version management system[2][5].Citations:
- 1: https://www.youtube.com/watch?v=qjiIA0220ms
- 2: https://github.com/golang/tools/releases
- 3: https://endoflife.date/go
- 4: https://go.dev/doc/devel/release
- 5: https://tip.golang.org/doc/go1.24
- 6: https://go.dev/dl/
- 7: https://tip.golang.org/doc/go1.23
- 8: https://antonz.org/which-go/
Go Version Compatibility Confirmed
Based on our verification, the
go.modfile specifyinggo 1.23.5is valid. The project intends to use the Go 1.23 series—and feedback from production confirms that a 1.23.x release works reliably. Although the latest stable release is Go 1.24.0, sticking with Go 1.23.5 remains an intentional, supported choice. There's no need to modify the version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/stack_processor_utils.go (1)
34-34: Linter issues with comment formattingThe static analysis tools indicate that these comment lines should end with a period.
- // Terraform section errors + // Terraform section errors.- // Helmfile section errors + // Helmfile section errors.- // Components section errors + // Components section errors.Also applies to: 46-46, 53-53
🧰 Tools
🪛 GitHub Check: golangci
[failure] 34-34: [golangci] internal/exec/stack_processor_utils.go#L34
Comment should end in a period (godot)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(13 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
internal/exec/stack_processor_utils.go
[failure] 34-34: [golangci] internal/exec/stack_processor_utils.go#L34
Comment should end in a period (godot)
[failure] 46-46: [golangci] internal/exec/stack_processor_utils.go#L46
Comment should end in a period (godot)
[failure] 53-53: [golangci] internal/exec/stack_processor_utils.go#L53
Comment should end in a period (godot)
[failure] 619-619: [golangci] internal/exec/stack_processor_utils.go#L619
add-constant: string literal "%s" appears, at least, 4 times, create a named constant for it (revive)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (6)
internal/exec/stack_processor_utils.go (6)
29-54: Well-structured error variables for improved error handlingThis adds standardized error variables for each section that can be invalid in the config file. The errors are logically grouped (Terraform section errors, Helmfile section errors, Components section errors) which makes the code more organized and reduces duplication.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 34-34: [golangci] internal/exec/stack_processor_utils.go#L34
Comment should end in a period (godot)
[failure] 46-46: [golangci] internal/exec/stack_processor_utils.go#L46
Comment should end in a period (godot)
[failure] 53-53: [golangci] internal/exec/stack_processor_utils.go#L53
Comment should end in a period (godot)
571-571: Added support for global hooks configurationThis new variable
globalHooksSectionenables hooks to be defined at the root configuration level, which matches the PR objective to "allow for hooks at multiple levels of config".
598-607: Enhanced error handling with proper error wrappingThe error handling has been improved by:
- Using the standardized error variables
- Wrapping errors with context using
errors.Wrapf- Including the stack name in error messages
This makes debugging easier by providing more context in error messages.
659-664: Added parsing and validation for global hooks sectionThis section properly validates the global hooks configuration, ensuring it's a valid map before using it.
671-675: Added merging of global hooks with terraform hooksThis implements the core functionality of merging hooks from different configuration levels (global and terraform-specific), fulfilling the PR objective.
1135-1146: Including global hooks in final component configurationThe final component hooks now incorporate both global hooks and terraform-specific hooks, ensuring that hooks defined at any level are included in the final configuration.
mcalhoun
force-pushed
the
bug/fix-store-bugs
branch
from
ea176c7 to
85800e3
Compare
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
503-505: Update of the filter_mode parameter in the lint-golangci step
Changing thefilter_modefrom"added"to"diff_context"allows the linter to consider the context around the changes, which should improve its feedback quality. This update appears aligned with the enhanced validation and error handling improvements in the PR.
📜 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 (1)
.github/workflows/test.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
mcalhoun
force-pushed
the
bug/fix-store-bugs
branch
from
7c9c36a to
c829fae
Compare
mcalhoun
force-pushed
the
bug/fix-store-bugs
branch
from
ca79dc8 to
d4cd5e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/store/artifactory_store_test.go (1)
50-50:⚠️ Potential issueFix file permission for security
The current permission (0o644) is too permissive for files that might contain sensitive data. Security best practices recommend permissions of 0o600 or less.
- if err := os.WriteFile(fullPath, data, 0o644); err != nil { + if err := os.WriteFile(fullPath, data, 0o600); err != nil {🧰 Tools
🪛 golangci-lint (1.62.2)
[medium] 50-50: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
🪛 GitHub Check: golangci
[failure] 50-50: [golangci] pkg/store/artifactory_store_test.go#L50
G306: Expect WriteFile permissions to be 0600 or less (gosec)
🧹 Nitpick comments (2)
internal/exec/stack_processor_utils.go (2)
575-580: Use wrapped static errors instead of dynamic errorsThe error creation follows the pattern used elsewhere in the codebase, but static analysis flags it as a potential issue. Consider using wrapped static errors for better error handling.
+var errInvalidHooksSection = errors.New("invalid 'hooks' section in the file") // Then in the function - return nil, fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName) + return nil, fmt.Errorf("%w: %s", errInvalidHooksSection, stackName)🧰 Tools
🪛 GitHub Check: golangci
[failure] 578-578: [golangci] internal/exec/stack_processor_utils.go#L578
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName)" (err113)
632-637: Apply consistent error handling approachSimilar to the above comment, consider using wrapped static errors here as well.
+var errInvalidTerraformHooksSection = errors.New("invalid 'terraform.hooks' section in the file") // Then in the function - return nil, fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName) + return nil, fmt.Errorf("%w: %s", errInvalidTerraformHooksSection, stackName)🧰 Tools
🪛 GitHub Check: golangci
[failure] 635-635: [golangci] internal/exec/stack_processor_utils.go#L635
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)internal/exec/stack_processor_utils.go(6 hunks)pkg/store/artifactory_store_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🪛 GitHub Check: golangci
internal/exec/stack_processor_utils.go
[failure] 578-578: [golangci] internal/exec/stack_processor_utils.go#L578
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'hooks' section in the file '%s'", stackName)" (err113)
[failure] 635-635: [golangci] internal/exec/stack_processor_utils.go#L635
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid 'terraform.hooks' section in the file '%s'", stackName)" (err113)
pkg/store/artifactory_store_test.go
[failure] 50-50: [golangci] pkg/store/artifactory_store_test.go#L50
G306: Expect WriteFile permissions to be 0600 or less (gosec)
🪛 golangci-lint (1.62.2)
pkg/store/artifactory_store_test.go
[medium] 50-50: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/store/artifactory_store_test.go (5)
44-44: Good update to octal notation formatThe change from
0755to0o755follows modern Go conventions for octal literals, making the code more explicit.
57-59: Method signature update correctly implementedSignature updated to include the new
optionsparameter of typeartifactory.UploadServiceOptionsand corresponding argument in the mock call.
148-168: Well-structured test cases for complex data typesGood addition of test cases for various data structures like slices, maps, and nested maps. These cases ensure the getKey function handles all expected data types correctly.
180-256: Comprehensive test coverage for Set operationThe new test function properly validates the
Setmethod with various data types and checks correct integration with mock client. The test structure mirrors the existing GetKey tests, maintaining consistency in the codebase.The mock expectations and assertions are well implemented, verifying both the function call parameters and success criteria.
243-249: Well-structured mock verificationGood use of
mock.MatchedByto create custom matchers for validating the parameters passed to the mock client. This approach ensures proper validation of complex parameters.internal/exec/stack_processor_utils.go (4)
544-544: Adding support for global hooks sectionThis new variable enables hooks to be configured at the root configuration level, supporting the PR objective to allow hooks at multiple levels.
644-647: Proper integration of global hooks with terraform hooksThis merge operation correctly combines global hooks with terraform-specific hooks, supporting the PR objective.
1108-1119: Hook merging properly prioritizes configurationsThe merge order now correctly includes the global hooks level, with proper precedence rules through the merge order:
- Global hooks first
- Terraform hooks second
- Base component hooks, component hooks, and overrides after
This implementation addresses the PR objective to allow hooks at multiple configuration levels.
1662-1662: Consistent formatting with trailing commasThis formatting change provides better consistency in function parameters and improves diff clarity in version control.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
|
These changes were released in v1.165.1. |
what
why
Summary by CodeRabbit
New Features
hookssection in theoverridesschema for enhanced customization of component behavior.Tests