fix(upgrades): upgrades when creating a new ledger on an already migration bucket #602
Conversation
WalkthroughThe changes in this pull request encompass extensive documentation updates in Changes
Suggested reviewers
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
CodeRabbit Configuration File (
|
| case errors.Is(err, system.ErrInvalidLedgerConfiguration{}) || | ||
| errors.Is(err, ledger.ErrInvalidLedgerName{}) || | ||
| errors.Is(err, system.ErrBucketUpgrading) || |
There was a problem hiding this comment.
you can put case toto, tata: instead of ||
There was a problem hiding this comment.
All switch at api level use the same pattern, I will change later, but not in this PR.
![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
🧹 Outside diff range and nitpick comments (2)
tools/generator/cmd/root.go (1)
158-170: Consider restructuring error handling for better clarity.The nested error handling structure could be simplified for better readability and maintainability.
Consider this alternative structure:
- if err != nil { - if !errors.As(err, &sdkError) || (sdkError.ErrorCode != components.V2ErrorsEnumLedgerAlreadyExists && - sdkError.ErrorCode != components.V2ErrorsEnumValidation) { - return fmt.Errorf("failed to create ledger: %w", err) - } - } + if err == nil { + return nil + } + var sdkError *sdkerrors.V2ErrorResponse + if !errors.As(err, &sdkError) { + return fmt.Errorf("failed to create ledger: %w", err) + } + if sdkError.ErrorCode != components.V2ErrorsEnumLedgerAlreadyExists && + sdkError.ErrorCode != components.V2ErrorsEnumValidation { + return fmt.Errorf("failed to create ledger: %w", err) + }internal/storage/driver/driver.go (1)
51-60: Consider adding debug logs for bucket stateThe bucket state checks are critical for understanding system behavior. Consider adding debug logs to help with troubleshooting.
if isInitialized { + logging.FromContext(ctx).Debugf("Bucket %s is initialized, checking if up to date", l.Bucket) upToDate, err := b.IsUpToDate(ctx) if err != nil { return nil, fmt.Errorf("checking if bucket is up to date: %w", err) } if !upToDate { + logging.FromContext(ctx).Debugf("Bucket %s is not up to date, returning ErrBucketUpgrading", l.Bucket) return nil, systemcontroller.ErrBucketUpgrading } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
internal/README.md(98 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/controller/system/errors.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(2 hunks)internal/storage/driver/buckets_generated_test.go(1 hunks)internal/storage/driver/driver.go(1 hunks)internal/storage/driver/driver_test.go(1 hunks)tools/generator/cmd/root.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/README.md
🔇 Additional comments (9)
internal/controller/system/errors.go (1)
10-10: LGTM: Error definition is clear and well-placed.
The new error variable ErrBucketUpgrading is appropriately defined and follows the existing error declaration pattern.
internal/storage/bucket/bucket.go (1)
18-18: Clarify the distinction between initialization checks.
The new IsInitialized method seems to overlap with the existing HasMinimalVersion method. Could you clarify the distinction between these methods to prevent confusion?
Let's check the implementation differences:
internal/api/v2/controllers_ledgers_create.go (1)
38-38: LGTM: Error handling is appropriate and consistent.
The addition of ErrBucketUpgrading to the validation errors group is logical, as it represents a temporary state where the request can be retried later. The BadRequest status code is appropriate for this scenario.
internal/api/v2/controllers_ledgers_create_test.go (1)
82-88: LGTM! Well-structured test case for bucket upgrade scenario.
The test case properly validates the error handling when attempting to create a ledger on an upgrading bucket. It follows the existing test pattern and has appropriate assertions.
tools/generator/cmd/root.go (1)
150-157: LGTM! Good addition of ledger existence check.
The addition of GetLedger before creation improves the robustness of the ledger creation process.
internal/storage/driver/buckets_generated_test.go (1)
85-98: LGTM! Generated mock implementation looks correct.
The generated mock implementation for IsInitialized follows the established pattern and correctly implements the interface.
internal/storage/driver/driver_test.go (1)
212-215: LGTM: Test coverage for bucket initialization check
The test correctly verifies the new initialization check before bucket migration.
internal/storage/driver/driver.go (1)
47-50: LGTM: Proper initialization check
The code properly checks if the bucket is initialized and handles errors appropriately.
internal/storage/bucket/default_bucket.go (1)
26-35: LGTM: Clean implementation of IsInitialized
The implementation correctly handles all cases:
- Existing version table
- Missing version table
- Other errors
gfyrag
force-pushed
the
fix/ledger-creation-on-upgrading-bucket
branch
from
079c13e to
a850d03
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/api/common/errors.go (1)
20-20: Consider adding documentation for the error constantTo improve maintainability, consider adding a comment explaining when this error occurs. This would help other developers understand the specific scenario where a bucket is considered outdated.
+// ErrBucketOutdated is returned when attempting to create a ledger on a bucket +// that requires migration or updates to meet the current version requirements ErrBucketOutdated = "BUCKET_OUTDATED"internal/storage/driver/driver.go (1)
47-60: LGTM: Good defensive checks added.The added validation steps help prevent issues with outdated buckets by:
- Checking if the bucket is initialized
- Verifying if an initialized bucket is up to date
- Returning appropriate errors in each case
Consider wrapping the error messages with more context about the bucket:
- return nil, fmt.Errorf("checking if bucket is initialized: %w", err) + return nil, fmt.Errorf("checking if bucket '%s' is initialized: %w", l.Bucket, err) - return nil, fmt.Errorf("checking if bucket is up to date: %w", err) + return nil, fmt.Errorf("checking if bucket '%s' is up to date: %w", l.Bucket, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yaml
📒 Files selected for processing (12)
docs/api/README.md(1 hunks)internal/README.md(98 hunks)internal/api/common/errors.go(1 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/controller/system/errors.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(2 hunks)internal/storage/driver/buckets_generated_test.go(1 hunks)internal/storage/driver/driver.go(1 hunks)internal/storage/driver/driver_test.go(1 hunks)tools/generator/cmd/root.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/controller/system/errors.go
- internal/storage/bucket/bucket.go
- internal/api/v2/controllers_ledgers_create.go
- internal/storage/bucket/default_bucket.go
- internal/api/v2/controllers_ledgers_create_test.go
- tools/generator/cmd/root.go
- internal/storage/driver/driver_test.go
👮 Files not reviewed due to content moderation or server errors (1)
- internal/storage/driver/buckets_generated_test.go
🔇 Additional comments (2)
internal/api/common/errors.go (1)
20-20: LGTM! Error constant follows conventions
The new error constant follows the established naming patterns and is appropriately placed with other ledger-related errors.
docs/api/README.md (1)
3247-3247: LGTM: Documentation updated to reflect new error code.
The addition of BUCKET_OUTDATED to the V2ErrorsEnum aligns with the code changes that introduce bucket version validation.
No description provided.