fix(operator): only delete ledger deployment on strategy change #1639
Conversation
Dav-14
commented
Aug 6, 2024
•
Dav-14
commented
- feat: add method lock/unlock on stack
- feat: delete deployment only on strategy update
|
Warning Rate limit exceeded@Dav-14 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 13 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates enhance the functionality and structure of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StackController
participant LedgerController
User->>StackController: Request stack operation
StackController->>Stack: Lock()
Stack->>Stack: Update annotations
StackController->>Stack: Unlock()
User->>LedgerController: Request ledger operation
LedgerController->>Ledger: Check deployment strategy
Ledger->>Ledger: Update status
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Dav-14
changed the title
![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
Outside diff range, codebase verification and nitpick comments (1)
components/operator/internal/resources/ledgers/deployments.go (1)
Line range hint
59-96: Ensure proper error handling in deferred function.While the deferred function updates the ledger conditions, it does not handle potential errors that might occur during this operation. Consider adding error handling to ensure robustness.
- defer func() { - ledger.GetConditions().AppendOrReplace(*cond, predicate) - }() + defer func() { + if err := ledger.GetConditions().AppendOrReplace(*cond, predicate); err != nil { + ctx.Logger().Error(err, "Failed to update ledger conditions") + } + }()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- components/operator/api/formance.com/v1beta1/stack_types.go (2 hunks)
- components/operator/internal/resources/ledgers/deployments.go (3 hunks)
- components/operator/internal/tests/ledger_controller_test.go (2 hunks)
- components/operator/internal/tests/stack_controller_test.go (2 hunks)
Additional comments not posted (9)
components/operator/api/formance.com/v1beta1/stack_types.go (3)
103-105: Relocation ofGetConditionsmethod is acceptable.The method's implementation remains unchanged, and its new location does not affect functionality.
111-116: Implementation ofLockmethod is correct.The method correctly initializes the
Annotationsmap if it is nil and sets theSkipLabelentry to "true".
118-119: Implementation ofUnLockmethod is correct.The method correctly removes the
SkipLabelentry from theAnnotationsmap.components/operator/internal/tests/stack_controller_test.go (2)
187-187: Usage ofstack.Lock()inBeforeEachblock is correct.The call to
stack.Lock()correctly replaces the previous annotation setup, aligning with the new locking mechanism.
227-227: Usage ofstack.UnLock()inJustBeforeEachblock is correct.The call to
stack.UnLock()correctly precedes the patch operation, ensuring proper sequence of operations.components/operator/internal/tests/ledger_controller_test.go (2)
209-215: New test case for strategy condition update is correct.The test case correctly verifies that the strategy condition is updated by checking the
ledger.Status.Conditions.
216-222: New test case for deployment deletion is correct.The test case correctly verifies the deletion of a deployment by ensuring that a specific resource cannot be successfully retrieved after the deletion operation.
components/operator/internal/resources/ledgers/deployments.go (2)
34-56: LGTM!The function
hasDeploymentStrategyChangedis well-structured and logically sound. It effectively checks the deployment strategy and handles the uninstallation of the ledger based on the strategy.
Line range hint
59-96: LGTM!The function
installLedgeris well-structured and logically sound. It effectively installs the ledger based on the deployment strategy and handles errors appropriately.
Dav-14
force-pushed
the
feat/eng-1259-fixoperator-no-drop-deployments-when-ledger-is-update
branch
from
00cf878
to
f6c7f39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- components/operator/api/formance.com/v1beta1/stack_types.go (2 hunks)
- components/operator/internal/resources/ledgers/deployments.go (3 hunks)
- components/operator/internal/tests/ledger_controller_test.go (3 hunks)
- components/operator/internal/tests/stack_controller_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- components/operator/api/formance.com/v1beta1/stack_types.go
- components/operator/internal/resources/ledgers/deployments.go
- components/operator/internal/tests/ledger_controller_test.go
Additional comments not posted (2)
components/operator/internal/tests/stack_controller_test.go (2)
187-187: Good use of locking mechanism.The
stack.Lock()call in theBeforeEachblock ensures that the stack is properly locked before performing any operations, enhancing the safety and reliability of concurrent access or state changes.
227-227: Good use of unlocking mechanism before patching.The
stack.UnLock()call before patching the stack ensures that the stack is properly unlocked, aligning with the intended use of locking mechanisms to manage concurrent access or state changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/operator/internal/resources/ledgers/deployments.go (3 hunks)
Additional comments not posted (3)
components/operator/internal/resources/ledgers/deployments.go (3)
28-32: LGTM!The new constants for deployment strategy conditions and reasons improve code readability and maintainability.
Line range hint
63-99:
LGTM! But verify the function usage in the codebase.The function implementation is approved.
However, ensure that all function calls to
installLedgerare correctly integrated.
34-60: LGTM! But verify the function usage in the codebase.The function implementation is approved.
However, ensure that all function calls to
hasDeploymentStrategyChangedare correctly integrated.Verification successful
The function
hasDeploymentStrategyChangedis correctly integrated within the filecomponents/operator/internal/resources/ledgers/deployments.go.
- The function is defined and used exclusively within this file, indicating proper encapsulation and usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `hasDeploymentStrategyChanged` are correctly integrated. # Test: Search for the function usage. Expect: Correct integration of the new function. rg --type go -A 5 $'hasDeploymentStrategyChanged'Length of output: 1363
Script:
#!/bin/bash # Description: Verify all function calls to `hasDeploymentStrategyChanged` are correctly integrated across the entire codebase. # Test: Search for the function usage across the codebase. Expect: Correct integration of the new function. rg --type go 'hasDeploymentStrategyChanged'Length of output: 422
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- components/operator/internal/resources/ledgers/deployments.go (2 hunks)
- components/operator/internal/tests/ledger_controller_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/operator/internal/tests/ledger_controller_test.go
Additional comments not posted (1)
components/operator/internal/resources/ledgers/deployments.go (1)
96-97: Add a default case in the switch statement.The switch statement for
deploymentStrategySettingscould benefit from a default case to handle unexpected values.switch deploymentStrategySettings { case v1beta1.DeploymentStrategySingle: return installLedgerSingleInstance(ctx, stack, ledger, database, image, isV2) case v1beta1.DeploymentStrategyMonoWriterMultipleReader: return installLedgerMonoWriterMultipleReader(ctx, stack, ledger, database, image, isV2) default: return fmt.Errorf("unknown deployment strategy %s", deploymentStrategySettings) }Likely invalid or redundant comment.
| if err = hasDeploymentStrategyChanged(ctx, stack, ledger, deploymentStrategySettings); err != nil { | ||
| return |
There was a problem hiding this comment.
Improve error handling for hasDeploymentStrategyChanged.
The error handling for hasDeploymentStrategyChanged could be more descriptive to provide better context in case of failure.
if err = hasDeploymentStrategyChanged(ctx, stack, ledger, deploymentStrategySettings); err != nil {
return fmt.Errorf("failed to check if deployment strategy changed: %w", err)
}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.
| if err = hasDeploymentStrategyChanged(ctx, stack, ledger, deploymentStrategySettings); err != nil { | |
| return | |
| if err = hasDeploymentStrategyChanged(ctx, stack, ledger, deploymentStrategySettings); err != nil { | |
| return fmt.Errorf("failed to check if deployment strategy changed: %w", err) |
Dav-14
force-pushed
the
feat/eng-1259-fixoperator-no-drop-deployments-when-ledger-is-update
branch
from
bdbaab3
to
ff4fbcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- components/operator/internal/resources/ledgers/deployments.go (2 hunks)
- components/operator/internal/tests/ledger_controller_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- components/operator/internal/resources/ledgers/deployments.go
- components/operator/internal/tests/ledger_controller_test.go
Dav-14
deleted the
feat/eng-1259-fixoperator-no-drop-deployments-when-ledger-is-update
branch
Co-authored-by: David Ragot <[email protected]>
Co-authored-by: David Ragot <[email protected]>
