-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
SidecarVersionChecker
variable 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
fetcher
variable 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
OracleClient
and 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
Stop
method 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
Version
method of theOracleClient
returns 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
Version
method 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
SlinkySidecarCheckDelay
and 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 forCheckSidecarVersion
To ensure the correctness of the version checking logic and to prevent regressions, consider adding unit tests for the
CheckSidecarVersion
method.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%w
in logging statements.The
%w
verb is used for error wrapping withfmt.Errorf
and is not supported in logging methods. Thec.logger.Error
method 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%w
in logging statements.Similar to previous instances,
%w
should not be used in theError
logging 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
SlinkyClientSidecarVersionFetcherDaemonModuleName
is 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
MinSidecarVersion
is 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
MinSidecarVersion
is 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
SlinkySidecarCheckDelay
is 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_test
for 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 callingStart
twice.The mock expectation for the
Start
method 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.Start
is 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
SidecarVersionChecker
aligns 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
AssetsKeeper
mock has been updated from./x//types
to./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
GetSidecarVersionHC
aligns 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.Error
call 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
NewTimeBoundedHealthCheckable
function call.The function call to
daemontypes.NewTimeBoundedHealthCheckable
seems 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.aaakk.us.kg/hashicorp/go-version v1.7.0
Please ensure that adding
github.com/hashicorp/go-version v1.7.0
as 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-go
packageThe package
github.com/dgrijalva/jwt-go
is deprecated and has known security vulnerabilities. Although it is being replaced withgithub.aaakk.us.kg/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 thereplace
directive.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-go
have been successfully removed from the source code. Additionally, the replace directive ingo.mod
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, 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