[TRA-721] Enforce sidecar versions #2491
Conversation
WalkthroughThe changes in this pull request primarily enhance the management and monitoring of daemon clients within the application. Key modifications include the introduction of health monitoring for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (6)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
protocol/mocks/SidecarVersionChecker.go (2)
16-55: LGTM: Well-implemented mock methods with a minor suggestion.The CheckSidecarVersion, Start, and Stop methods are correctly implemented following the testify mock pattern. They properly record method calls and handle return values.
Consider adding comments to each method explaining their purpose and expected behavior in the context of the SidecarVersionChecker interface. This would improve the readability and maintainability of the mock.
1-69: Overall: Well-implemented mock for SidecarVersionChecker.This auto-generated mock file for SidecarVersionChecker is well-structured and follows best practices for testify mocks. It includes all necessary components: a properly defined struct, correctly implemented methods (CheckSidecarVersion, Start, and Stop), and a constructor function with appropriate cleanup. The implementation should serve its purpose well in testing scenarios.
To further improve the testing infrastructure:
- Ensure that the actual SidecarVersionChecker interface (which this mock implements) is well-documented in its original location.
- Consider creating usage examples or helper functions for common testing scenarios using this mock, which could be placed in a separate test utility file.
protocol/daemons/slinky/client/sidecar_version_checker_test.go (4)
16-19: LGTM: Test function setup is clear and concise.The test function is well-named and the logger setup is appropriate. The
SidecarVersionCheckervariable declaration is fine, but consider adding a comment explaining why it's declared here if it's intended to be used across subtests.Consider adding a comment above the
fetchervariable declaration to explain its purpose and usage across subtests.
20-32: LGTM: "Checks sidecar version passes" test case is well-structured.The test case effectively mocks the
OracleClientand sets up the correct expectations. It properly tests the scenario where the sidecar version matches the minimum required version.Consider adding an assertion to verify that the
Stopmethod was called on the mockOracleClient. This can be done usingslinky.AssertExpectations(t)at the end of the test.
34-61: LGTM: Error scenario test cases are comprehensive and well-structured.Both "Checks sidecar version less than minimum version" and "Checks sidecar version incorrectly formatted" test cases effectively cover important error scenarios. The error message checks are appropriate and the overall structure is consistent with the first test case.
For consistency, consider adding
slinky.AssertExpectations(t)at the end of each test case, similar to the suggestion for the first test case. This ensures that all expected methods on the mock were called as anticipated.
1-62: Overall, excellent test coverage and structure for SidecarVersionChecker.The test file provides comprehensive coverage for the
SidecarVersionChecker, including both success and error scenarios. The use of subtests, mocking, and clear assertions contributes to high-quality, maintainable tests.To further enhance the tests, consider:
- Adding edge cases, such as testing with a version slightly higher than the minimum required version.
- Testing the behavior when the
Versionmethod of theOracleClientreturns an error.- Extracting common setup code (like creating the mock
OracleClient) into a helper function to reduce duplication across test cases.protocol/daemons/slinky/client/client_test.go (2)
63-66: LGTM: Version checking implemented for Slinky client.The addition of the
Versionmethod to the mock is a good improvement. It suggests that version checking has been implemented for the Slinky client, which can help ensure compatibility between the client and the sidecar.Consider adding tests to verify the behavior when the sidecar version is below
MinSidecarVersion.
70-70: LGTM: Sidecar version health check implemented.The addition of
SlinkySidecarCheckDelayand the inclusion ofGetSidecarVersionHC().HealthCheck()in the health check process are good improvements. They enhance the overall health monitoring of the system by ensuring the sidecar version is also checked.Consider adding a comment explaining why the delay is set to 1 millisecond, to clarify that this is specifically for testing purposes and not intended for production use.
Also applies to: 82-84
protocol/daemons/slinky/client/sidecar_version_checker.go (1)
47-72: Consider adding unit tests forCheckSidecarVersionTo ensure the correctness of the version checking logic and to prevent regressions, consider adding unit tests for the
CheckSidecarVersionmethod.Would you like assistance in generating the unit tests for this method?
protocol/daemons/slinky/client/client.go (2)
Line range hint
87-89: Incorrect use of%win logging statements.The
%wverb is used for error wrapping withfmt.Errorfand is not supported in logging methods. Thec.logger.Errormethod does not interpret format verbs like%w. Instead, pass the error as a separate argument.Apply this diff to fix the logging statement:
- c.logger.Error("Error initializing PriceFetcher in slinky daemon: %w", err) + c.logger.Error("Error initializing PriceFetcher in slinky daemon", "error", err)
Line range hint
124-126: Incorrect use of%win logging statements.Similar to previous instances,
%wshould not be used in theErrorlogging method.Apply this diff:
- c.logger.Error("Error initializing MarketPairFetcher in slinky daemon: %w", err) + c.logger.Error("Error initializing MarketPairFetcher in slinky daemon", "error", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- protocol/app/app.go (1 hunks)
- protocol/daemons/slinky/client/client.go (6 hunks)
- protocol/daemons/slinky/client/client_test.go (2 hunks)
- protocol/daemons/slinky/client/constants.go (1 hunks)
- protocol/daemons/slinky/client/sidecar_version_checker.go (1 hunks)
- protocol/daemons/slinky/client/sidecar_version_checker_test.go (1 hunks)
- protocol/go.mod (1 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/mocks/SidecarVersionChecker.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (20)
protocol/daemons/slinky/client/constants.go (5)
23-23: LGTM. Consistent naming for the new module.The addition of
SlinkyClientSidecarVersionFetcherDaemonModuleNameis appropriate and follows the existing naming convention for module names. This will help in identifying logs related to the sidecar version fetcher daemon.
20-22: LGTM. Improved formatting for consistency.The reformatting of the existing constants improves readability and maintains consistency across all constants in the file. This change is purely cosmetic and doesn't affect functionality.
15-24: Summary of changes: Sidecar version enforcement implementedThese changes introduce new variables and constants to support sidecar version checking and improve the overall consistency of the file. The additions align with the PR objective of enforcing sidecar versions. Key points:
- A new delay for sidecar checks is introduced (
SlinkySidecarCheckDelay).- A new module name for the sidecar version fetcher daemon is added.
- A minimum sidecar version is specified.
- Existing constants are reformatted for better readability.
These changes lay the groundwork for implementing sidecar version enforcement in the Slinky client. Ensure that the introduced values (check delay and minimum version) are appropriate for your use case.
24-24: LGTM. Verify the minimum sidecar version.The addition of
MinSidecarVersionis appropriate for specifying the minimum required version for the sidecar. Please ensure that version "v1.0.12" is compatible with the current codebase and doesn't introduce any breaking changes.Let's check for any related discussions or documentation about the minimum sidecar version:
✅ Verification successful
LGTM. Minimum sidecar version verified.
The addition of
MinSidecarVersionis appropriate and has been verified against the codebase to ensure compatibility with version "v1.0.12".🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related discussions or documentation about minimum sidecar version rg --type go -i "min.*sidecar.*version" rg --type md -i "minimum.*sidecar.*version|sidecar.*compatibility"Length of output: 516
15-15: LGTM. Verify the sidecar check delay.The addition of
SlinkySidecarCheckDelayis appropriate for controlling the frequency of sidecar checks. The 60-second delay seems reasonable, but please verify that this aligns with the intended check frequency for the sidecar.To ensure this delay is appropriate, let's check for any related configurations or discussions:
protocol/mocks/SidecarVersionChecker.go (3)
1-3: LGTM: Appropriate file header and package declaration.The file header correctly indicates that this is auto-generated code, and the package name "mocks" is suitable for a mock implementation.
5-14: LGTM: Correct imports and struct definition.The import statements include the necessary packages for context and mocking. The SidecarVersionChecker struct is correctly defined with the embedded mock.Mock, which is standard for testify mocks.
57-69: LGTM: Well-implemented constructor with proper cleanup.The NewSidecarVersionChecker function is correctly implemented as a constructor for the mock. It takes an appropriate testing interface, creates the mock instance, and sets up a cleanup function to assert expectations after the test. This follows best practices for testify mocks.
protocol/daemons/slinky/client/sidecar_version_checker_test.go (1)
1-14: LGTM: Package declaration and imports are appropriate.The package is correctly declared as
client_testfor external tests, and all necessary imports for testing and mocking are present.protocol/daemons/slinky/client/client_test.go (2)
Line range hint
1-87: Overall, the changes improve the Slinky client tests.The modifications to this test file enhance the robustness of the Slinky client tests by:
- Implementing version checking for the sidecar.
- Expanding the health check process to include sidecar version verification.
- Adjusting the mock expectations to match new behavior.
These changes will help ensure better compatibility and reliability of the Slinky client. The suggestions provided in the previous comments should be addressed to further improve the clarity and completeness of the tests.
55-55: Verify the intention behind callingStarttwice.The mock expectation for the
Startmethod has been changed from.Once()to.Twice(). This suggests that the Slinky service is now being started twice in the client's lifecycle. Can you please clarify why this change was made? It's unusual for a service to be started multiple times, so we should ensure this is intentional and not a potential issue.✅ Verification successful
The
.Twice()expectation forslinky.Startis valid, as the method is called twice across different components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for occurrences of slinky.Start() in the codebase rg --type go 'slinky\.Start\(' -A 5 -B 5Length of output: 1765
protocol/mocks/Makefile (2)
63-63: New mock added for SidecarVersionCheckerThe addition of a mock for
SidecarVersionCheckeraligns with the PR objective of enforcing sidecar versions. This new mock will enable better testing of the version checking functionality in the Slinky daemon.
67-67: Path correction for AssetsKeeper mockThe directory path for generating the
AssetsKeepermock has been updated from./x//typesto./x/assets/types. This correction improves the accuracy of the mock generation process and reflects the correct module structure.protocol/daemons/slinky/client/client.go (4)
67-69: Consistent accessor methods for health checks.The addition of
GetSidecarVersionHCaligns with the existing pattern for health check accessors. Ensure that this method is used wherever necessary to maintain consistency.
187-189: Consistent error handling in sidecar version check.The error message in the
c.logger.Errorcall is clear, and the health check correctly reports failure. Ensure that the error wrapping provides meaningful context.
172-196: Proper context cancellation handling inRunSidecarVersionChecker.The function correctly handles context cancellation and ensures resources are cleaned up properly upon termination.
48-52:⚠️ Potential issueMissing trailing argument in
NewTimeBoundedHealthCheckablefunction call.The function call to
daemontypes.NewTimeBoundedHealthCheckableseems to have a missing argument or an extra trailing comma, which could lead to a syntax error.Apply this diff to fix the syntax:
sidecarVersionHC: daemontypes.NewTimeBoundedHealthCheckable( SlinkyClientSidecarVersionFetcherDaemonModuleName, &libtime.TimeProviderImpl{}, logger, - ), + )Likely invalid or redundant comment.
protocol/go.mod (2)
65-65: Verify the addition ofgithub.com/hashicorp/go-version v1.7.0Please ensure that adding
github.com/hashicorp/go-version v1.7.0as a direct dependency is necessary. Verify that this dependency is being used appropriately in the codebase and does not introduce any conflicts with existing packages.Run the following script to locate usages of
github.com/hashicorp/go-version:
Line range hint
258-261: Confirm removal of deprecateddgrijalva/jwt-gopackageThe package
github.com/dgrijalva/jwt-gois deprecated and has known security vulnerabilities. Although it is being replaced withgithub.com/golang-jwt/jwt/v4, please verify that all references to the deprecated package have been fully removed from the codebase. If it is no longer used, consider removing thereplacedirective.Run the following script to check for any remaining imports of the deprecated package:
✅ Verification successful
Deprecation Confirmed and Replace Directive Can Be Removed
All references to
github.com/dgrijalva/jwt-gohave been successfully removed from the source code. Additionally, the replace directive ingo.modis no longer necessary and can be safely removed to eliminate the deprecated package entirely.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate any imports of 'github.com/dgrijalva/jwt-go' in the codebase. # Expected: No occurrences of the deprecated package. rg --type go 'import.*"github.com/dgrijalva/jwt-go"'Length of output: 54
Script:
#!/bin/bash # Description: Locate any references to 'github.com/dgrijalva/jwt-go' in the entire codebase, including go.mod. # Expected: No occurrences of the deprecated package in any files. rg 'github.com/dgrijalva/jwt-go'Length of output: 116
Script:
#!/bin/bash # Description: List all replace directives in go.mod to identify remaining references. grep '^replace ' protocol/go.modLength of output: 64
protocol/app/app.go (1)
891-891: LGTM!The additional health check for the sidecar version is correctly registered, enhancing the application's health monitoring capabilities.
| sidecarVersionChecker SidecarVersionChecker | ||
| sidecarVersionHC daemontypes.HealthCheckable |
There was a problem hiding this comment.
Ensure sidecarVersionChecker is initialized in newClient.
While sidecarVersionHC is initialized in the newClient function, the sidecarVersionChecker field is not. This could lead to a nil pointer dereference when accessing sidecarVersionChecker before it is initialized in the start method. Consider initializing sidecarVersionChecker in the newClient function or ensuring it's safely used only after initialization.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores