chore: errors block processing simplification #4046
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes modify the error configuration handling by enhancing deep copy and merge functionalities. In Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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
|
| clone.Retry[i] = retry.Clone() | ||
| // Merge combines the current ErrorsConfig with another one, prioritizing the other config | ||
| func (c *ErrorsConfig) Merge(other *ErrorsConfig) { | ||
| if c == nil || other == nil { |
There was a problem hiding this comment.
If you merge a config into a nil config, shouldn't you expect that c becomes other?
There was a problem hiding this comment.
Since changes to the receiver don't affect the caller, initializing c has no outside effect - better to return early if null.
config/errors_block.go
Outdated
| if otherBlock.SleepIntervalSec > 0 { | ||
| existingBlock.SleepIntervalSec = otherBlock.SleepIntervalSec | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Consider replacing the else with a continue to dodge the nest here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
util/collections.go (1)
228-244: Clarify doc about Go map iteration order
The doc comment implies that the slice preserves an original order from the map. However, Go maps do not guarantee a stable iteration order across runs. Consider updating the comment to reflect that the function simply appends values in the order they are encountered, which is not deterministic.-// It extracts all values from the map and returns them as a slice while maintaining their original order in the map iteration. +// It extracts all values from the map and returns them as a slice in the order +// encountered during iteration, which is not guaranteed to remain consistent +// across runs due to Go's non-deterministic map iteration.config/errors_block.go (1)
44-52: In-place merge may cause side effects
TheMergemethod mutates the originalErrorsConfiginstead of returning a new merged configuration. If this is intentional, it’s fine. Otherwise, returning a new copy might help avoid unintended modifications.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/errors_block.go(2 hunks)util/collections.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
util/collections.goconfig/errors_block.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (10)
config/errors_block.go (10)
12-13: Struct fields look good
TheRetryandIgnorefields are clearly declared, using appropriate struct tags forctyandhcl.
32-42: Ensure coverage for deep clone functionality
This approach correctly deep-clonesErrorsConfigby cloning its constituent slices. Consider verifying in tests by mutating clones to confirm that no unintended references remain.
54-66: Clone logic for RetryBlock
Implementation is straightforward, ensuring a proper deep copy of all fields.
68-80: Clone logic for IgnoreBlock
Method mirrors theRetryBlocklogic. Looks correct for deep copying all fields, including the signals map.
82-94: Deep copy of RetryBlock slices
This helper correctly invokesClone()on eachRetryBlock, ensuring no references to the original slice.
96-108: Deep copy of IgnoreBlock slices
Implementation parallelscloneRetryBlocks; no issues spotted.
110-120: Cloning string slices
Simple copy approach usingmakeandcopyis correct.
122-132: Cloning signals map
Usingmaps.Copyis appropriate for a deep copy without referencing the original map.
134-161: Zero-values not overridden
InmergeRetryBlocks, zero values forMaxAttemptsandSleepIntervalSecare skipped. Confirm whether ignoring an explicit zero in the other slice is intended or if you need to allow overriding with zero.
163-195: Empty messages not merged
InmergeIgnoreBlocks, you skip assigning an empty message to overwrite an existing one. Verify if this is desired or if you want an empty string to clear an old message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/errors_block.go (2)
143-158: Consider fixing code style in the mergeRetryBlocks functionThe function logic is correct, but there's a minor style issue with branch statement cuddling. According to static analysis hints, branch statements should not be cuddled if the block has more than two lines.
if otherBlock.SleepIntervalSec > 0 { existingBlock.SleepIntervalSec = otherBlock.SleepIntervalSec } + continue } retryMap[otherBlock.Label] = otherBlock🧰 Tools
🪛 golangci-lint (1.64.8)
155-155: branch statements should not be cuddled if block has more than two lines
(wsl)
157-157: assignments should only be cuddled with other assignments
(wsl)
172-191: Consider using continue instead of else in mergeIgnoreBlocksFor consistency with the mergeRetryBlocks implementation and to reduce nesting, consider using continue instead of else here.
if existingBlock, found := ignoreMap[otherBlock.Label]; found { existingBlock.IgnorableErrors = util.MergeStringSlices(existingBlock.IgnorableErrors, otherBlock.IgnorableErrors) if otherBlock.Message != "" { existingBlock.Message = otherBlock.Message } if otherBlock.Signals != nil { if existingBlock.Signals == nil { existingBlock.Signals = make(map[string]cty.Value, len(otherBlock.Signals)) } maps.Copy(existingBlock.Signals, otherBlock.Signals) } + continue - } else { - ignoreMap[otherBlock.Label] = otherBlock - } + } + ignoreMap[otherBlock.Label] = otherBlock
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(0 hunks)config/errors_block.go(2 hunks)
💤 Files with no reviewable changes (1)
- config/config.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
config/errors_block.go
🧬 Code Definitions (1)
config/errors_block.go (2)
options/options.go (1)
ErrorsConfig(654-657)util/collections.go (2)
MergeStringSlices(214-227)MapToSlice(237-244)
🪛 golangci-lint (1.64.8)
config/errors_block.go
155-155: branch statements should not be cuddled if block has more than two lines
(wsl)
157-157: assignments should only be cuddled with other assignments
(wsl)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (10)
config/errors_block.go (10)
32-41: The Clone method implementation is well structuredGood use of nil checking and the new helper methods for deep copying the Retry and Ignore fields. This ensures a proper deep copy while maintaining nil check safety.
44-52: The Merge method handles nil receivers appropriatelyThe early return for nil cases is a good practice. Since we're merging configurations, checking that both the receiver and argument are non-nil prevents unexpected behavior.
54-66: The RetryBlock.Clone method is well implementedThe method correctly handles nil receivers and performs a proper deep copy of the RetryableErrors slice using the cloneStringSlice helper function.
68-80: The IgnoreBlock.Clone method is correctly implementedGood implementation with nil checking and proper deep copying of all fields, particularly the Signals map using the cloneSignalsMap helper.
82-94: The cloneRetryBlocks helper function is well structuredThis helper effectively creates a deep copy of a slice of RetryBlock pointers. The nil check at the beginning is appropriate, and the implementation is clear and concise.
96-108: The cloneIgnoreBlocks helper function is well implementedSimilar to cloneRetryBlocks, this function properly handles nil inputs and creates a deep copy of each IgnoreBlock in the slice.
110-120: The cloneStringSlice helper function is efficientGood use of the built-in copy function to efficiently duplicate the string slice while preserving nil behavior.
122-132: The cloneSignalsMap helper function is correctly implementedGood use of maps.Copy for efficient copying of the map contents. The nil check provides proper safety for edge cases.
134-161: Good implementation of merging logic for RetryBlocksThe implementation efficiently uses a map for lookups and correctly prioritizes values from the other RetryBlocks while preserving existing values where appropriate. The use of MapToSlice for the final conversion is clean.
🧰 Tools
🪛 golangci-lint (1.64.8)
155-155: branch statements should not be cuddled if block has more than two lines
(wsl)
157-157: assignments should only be cuddled with other assignments
(wsl)
163-195: Good implementation of merging logic for IgnoreBlocksThe merging logic correctly handles all IgnoreBlock fields, with proper handling of nil maps and prioritization of the other configuration. The use of MapToSlice is consistent with the RetryBlocks implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
util/collections_test.go (1)
277-298: Good test implementation with room for enhancement.The test function follows good practices with parallel execution and proper edge case handling. However, consider these improvements:
- Add a test case for a multi-element map to ensure the function works correctly in that scenario.
- Use the
assertpackage that's already imported and used elsewhere in the file for consistency instead of manual error reporting witht.Errorf.func TestMapToSlice(t *testing.T) { t.Parallel() t.Run("Empty Map", func(t *testing.T) { t.Parallel() m := make(map[string]*int) result := util.MapToSlice(m) - if len(result) != 0 { - t.Errorf("Expected empty slice, got %v", result) - } + assert.Empty(t, result, "Expected empty slice") }) t.Run("Single Element Map", func(t *testing.T) { t.Parallel() val := 42 m := map[string]*int{"key1": &val} result := util.MapToSlice(m) - if len(result) != 1 || result[0] != &val { - t.Errorf("Expected slice with one element %v, got %v", &val, result) - } + assert.Len(t, result, 1, "Expected slice with one element") + assert.Equal(t, &val, result[0], "Element in slice should match the map value") }) + t.Run("Multi Element Map", func(t *testing.T) { + t.Parallel() + val1, val2 := 42, 24 + m := map[string]*int{"key1": &val1, "key2": &val2} + result := util.MapToSlice(m) + assert.Len(t, result, 2, "Expected slice with two elements") + // Since map iteration order is not guaranteed, we check that both values are present + values := []*int{&val1, &val2} + assert.Contains(t, values, result[0], "First element should be one of the map values") + assert.Contains(t, values, result[1], "Second element should be one of the map values") + assert.NotEqual(t, result[0], result[1], "Elements should be different") + }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(0 hunks)util/collections_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- config/config.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
util/collections_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
Description
Simplified code for processing errors block
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests
MapToSlicefunction, validating behavior with empty and single-element maps.