feat(tools/cosmovisor): create current symlink as relative #21891
Conversation
troian
requested review from
julienrbrt,
facundomedica and
a team
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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
Documentation and Community
|
troian
force-pushed
the
cosmovisor-relative-symlink
branch
2 times, most recently
from
d5caf27
to
1d4b3d3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (8)
tools/cosmovisor/args.go (1)
135-156: Approve changes with suggestions for code cleanupThe updates to the CurrentBin method improve symlink handling and error checking. The use of
os.Lstatinstead ofos.Statis more appropriate for symlink verification.Consider the following improvements:
- Remove the commented-out code (lines 140-143) to maintain code cleanliness.
- Add a comment explaining why
os.Lstatis used instead ofos.Statfor better code documentation.Here's a suggested diff for these changes:
info, err := os.Lstat(cur) if err != nil || (info.Mode()&os.ModeSymlink == 0) { - // err = os.Remove(cur) - // if err != nil { - // return "", err - // } + // If not a symlink, create a symlink to genesis // Create symlink to the genesis return cfg.SymLinkToGenesis() } + // Use os.Lstat to get information about the symlink itself, not the file it points to _, err = os.Readlink(cur) if err != nil { // Create symlink to the genesis return cfg.SymLinkToGenesis() }These changes will improve code readability and maintainability.
tools/cosmovisor/cmd/cosmovisor/init_test.go (2)
217-217: Consider removing the empty line for consistency.While the added empty line doesn't affect functionality, it's not consistent with the surrounding code style. Consider removing it to maintain a uniform code structure.
-
242-242: Consider removing the empty line for consistency.Similar to the previous comment, this added empty line doesn't affect functionality but isn't consistent with the surrounding code style. Consider removing it to maintain a uniform code structure.
-CHANGELOG.md (5)
Line range hint
1-98: Ensure all unreleased changes are properly documentedThe unreleased changes section provides a good overview of the upcoming features, improvements, and bug fixes. However, consider the following suggestions to enhance the clarity and usefulness of the changelog:
- Add a brief summary at the beginning of the unreleased section to highlight the most significant changes.
- Consider grouping related changes together, especially for cross-cutting concerns like performance improvements or API changes.
- Ensure that each entry provides enough context for users who may not be familiar with the internal workings of the project.
- For bug fixes, consider adding information about the impact of the bug and how it affects users.
Line range hint
100-124: Enhance feature descriptions for better user understandingThe Features section provides a good overview of new additions to the SDK. To improve its usefulness:
- Consider adding brief explanations of the impact or benefits of each feature for end-users or developers.
- Ensure consistency in the level of detail provided for each feature. Some entries are very detailed, while others are quite brief.
- For features that introduce new concepts or significant changes, consider adding links to relevant documentation or examples.
Line range hint
126-170: Clarify the impact of improvements for users and developersThe Improvements section provides a good overview of enhancements to the SDK. To make it more useful:
- For each improvement, consider adding a brief note on how it benefits users or developers. This helps readers understand the significance of the change.
- Ensure consistency in the level of technical detail provided across entries. Some are quite technical, while others are more general.
- For improvements that change existing behavior, clearly indicate any potential impact on backwards compatibility or required actions from users.
- Consider grouping related improvements together to provide a clearer picture of overall enhancements in specific areas of the SDK.
Line range hint
172-185: Enhance bug fix descriptions for better contextThe Bug Fixes section provides valuable information about resolved issues. To improve its usefulness:
- For each bug fix, consider adding a brief description of how the bug might have affected users. This helps readers understand the importance of the fix.
- Ensure consistency in providing links to related issues or pull requests for all entries.
- Where applicable, mention any potential impact on performance or behavior that users should be aware of after applying the fix.
- Consider grouping related bug fixes together, especially if they address different aspects of the same underlying issue.
Line range hint
1-185: Overall improvement suggestions for the CHANGELOGThe CHANGELOG.md file provides a comprehensive overview of the changes, improvements, and bug fixes in the Cosmos SDK. To further enhance its usefulness:
- Consider adding a "Breaking Changes" section at the top to highlight any changes that may require action from users or potentially break existing implementations.
- Maintain consistency in the level of detail and style across all entries.
- Where possible, provide links to relevant documentation, examples, or discussions for significant changes or new features.
- Consider adding a "Deprecations" section to clearly communicate any features or APIs that are being phased out.
- For each release, consider adding a brief summary of the most significant changes or overarching themes to give readers a quick overview.
These improvements will help users and developers quickly understand the changes and their potential impact when updating to new versions of the SDK.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between f6d7a92 and 1d4b3d379189c06551a43b492c7881b23325d3e5.
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (3 hunks)
- tools/cosmovisor/cmd/cosmovisor/init.go (2 hunks)
- tools/cosmovisor/cmd/cosmovisor/init_test.go (9 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/run.go (2 hunks)
Additional context used
Path-based instructions (6)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/init.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/init_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/root.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/run.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (10)
tools/cosmovisor/cmd/cosmovisor/run.go (1)
43-47: LGTM! Consider verifying subsequent operations.The new code block successfully implements the PR objective by changing the working directory to allow for relative symlinks. The error handling is appropriate, and the comment clearly explains the purpose.
To ensure this change doesn't negatively impact subsequent operations, please run the following verification:
This will help identify any potential issues with file operations or path manipulations that might be affected by the directory change.
Verification successful
Verified: Subsequent operations are compatible with the directory change.
No file operations or path manipulations were detected after changing the working directory, ensuring that the directory change does not negatively impact subsequent functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that subsequent operations in the run function are compatible with the directory change. # Test: Search for file operations or path manipulations after the directory change. rg --type go -A 10 -B 2 'os.Chdir\(cfg.Root\(\)\)' tools/cosmovisor/cmd/cosmovisor/run.goLength of output: 466
tools/cosmovisor/cmd/cosmovisor/init.go (1)
17-17: Improved function naming:NewIntCmdtoNewInitCmdThe change from
NewIntCmdtoNewInitCmdis a good improvement. It more accurately describes the function's purpose of initializing a cosmovisor daemon home directory, enhancing code readability and maintainability.tools/cosmovisor/args.go (1)
382-399:⚠️ Potential issueApprove changes with a suggestion for path construction
The updates to the SetCurrentUpgrade method improve symlink handling by using relative paths, which enhances portability. However, there's a potential issue with the upgrade info file path construction.
The path construction for the upgrade info file (line 399) now includes
cfg.Root(), which might lead to an incorrect path. Consider removingcfg.Root()from this path construction, as theupgradevariable already contains the relative path.Here's a suggested fix:
- f, err := os.Create(filepath.Join(cfg.Root(), upgrade, upgradetypes.UpgradeInfoFilename)) + f, err := os.Create(filepath.Join(upgrade, upgradetypes.UpgradeInfoFilename))This change ensures that the upgrade info file is created in the correct location relative to the current working directory.
To verify the correct path construction, you can run the following script:
#!/bin/bash # Description: Verify the upgrade info file path construction # Test: Check the structure of the upgrades directory ls -R upgradesThis script will help ensure that the upgrade info file is being created in the correct location within the upgrades directory structure.
tools/cosmovisor/cmd/cosmovisor/init_test.go (7)
141-142: LGTM: Correct interface implementation assertions.The added assertions correctly ensure that
BufferedPipeimplements bothio.Readerandio.Writerinterfaces using pointer receivers. This is a good practice for type checking and documentation.
187-187: LGTM: Consistent return type for StartNewBufferedPipe.The function signature change to return a pointer to
BufferedPipeis consistent with the changes made toNewBufferedPipe, maintaining API consistency.
252-252: LGTM: Correct use of pointer receivers for Read and Write methods.The change to use pointer receivers for the
ReadandWritemethods is correct and consistent with the earlier interface assertions. This improves efficiency and allows direct modification of theBufferedPipeinstance.Also applies to: 257-257
279-279: LGTM: Consistent return of BufferedPipe pointer.This change is consistent with the earlier modifications to
NewBufferedPipe, now correctly returning the pointer without using the address-of operator.
454-454: LGTM: Improved error message specificity.The updated error message provides more specific information about the failure, which is helpful for debugging and understanding the issue.
696-698: LGTM: Proper resource management with deferred file closing.The addition of a deferred file closing function is a good practice to ensure that resources are properly released, even in the event of an error.
170-171: LGTM: Consistent return type for NewBufferedPipe.The function signature change to return a pointer to
BufferedPipeis consistent with the earlier interface assertions and is a good practice for larger structs.Let's verify that this change is consistently applied throughout the codebase:
Verification successful
Verified: All usages of
NewBufferedPipehandle it as a pointer, ensuring consistency throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for NewBufferedPipe function calls and ensure they're handled as pointers rg --type go 'NewBufferedPipe\(' -A 3 -B 1Length of output: 1742
troian
changed the title
troian
force-pushed
the
cosmovisor-relative-symlink
branch
2 times, most recently
from
a3eed59
to
bb72f63
Compare
|
converting to draft until WIP is removed |
required by cosmos#21891 allows cosmovisor tests to natively run on following os/arch pairs linux/amd64, linux/arm64 darwin/amd64, darwin/arm64 Signed-off-by: Artur Troian <[email protected]>
troian
force-pushed
the
cosmovisor-relative-symlink
branch
2 times, most recently
from
f932209
to
ca6739c
Compare
troian
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tools/cosmovisor/testdata/repo/chain2-zip_bin/autod (1)
13-13: Simplifiedinfofield improves flexibility and maintainability.The change to the
infofield is a good improvement:
- It simplifies the JSON structure by replacing embedded data with a URL.
- The URL includes a SHA256 checksum, enhancing security.
- This approach allows for easier updates to the referenced data without changing this script.
- It aligns with the PR objective of improving Cosmovisor's functionality, potentially addressing Docker-related issues.
Consider documenting this change in the Cosmovisor documentation, explaining the new format of the
infofield and how it affects the upgrade process.tools/cosmovisor/upgrade_test.go (1)
266-277: Introduction ofprepareConfighelper functionThe addition of the
prepareConfigfunction is a great improvement:
- It centralizes common setup logic used across multiple tests, enhancing code reusability and maintainability.
- The function encapsulates the creation of a temporary directory and configuration setup, simplifying test implementations.
However, changing the working directory (line 273-274) in a test helper function could potentially affect other tests if not properly managed. Consider using a defer statement to change back to the original directory, or document this behavior clearly for other developers.
Consider adding a defer statement to revert the working directory change:
func prepareConfig(t *testing.T, testData string, config cosmovisor.Config) *cosmovisor.Config { t.Helper() tmpdir := copyTestData(t, testData) config.Home = tmpdir + originalDir, err := os.Getwd() + require.NoError(t, err) err := os.Chdir(config.Root()) require.NoError(t, err) + t.Cleanup(func() { + err := os.Chdir(originalDir) + require.NoError(t, err) + }) return &config }tools/cosmovisor/process_test.go (2)
23-27: workDir initialization looks good, but consider a more robust approachThe addition of the
workDirvariable and its initialization in theinit()function is a good way to centralize the working directory information. However, usingos.Getwd()might cause issues if the working directory changes during test execution.Consider using a more robust approach, such as:
- Setting the working directory explicitly at the beginning of each test.
- Using a fixed path relative to the test file location.
This would ensure consistency across all tests regardless of the current working directory.
33-41: Improved test setup and binary path comparisonsThe changes in this test function enhance its robustness and maintainability:
- The use of
prepareConfigcentralizes the configuration setup.- Using
workDirimproves portability across different environments.- Resolving symbolic links with
filepath.EvalSymlinksensures more accurate binary path comparisons.These changes are approved. However, consider adding a comment explaining the purpose of
filepath.EvalSymlinksfor better code readability.Consider adding a comment like:
// Resolve symbolic links to ensure accurate binary path comparisons rPath, err := filepath.EvalSymlinks(cfg.GenesisBin())Also applies to: 51-53, 72-75, 87-90
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 1d4b3d379189c06551a43b492c7881b23325d3e5 and ca6739c6955dd10db7cc47cb045c013c09f3d3c4.
⛔ Files ignored due to path filters (1)
tools/cosmovisor/testdata/repo/chain2-zip_bin/autod.zipis excluded by!**/*.zip
📒 Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (3 hunks)
- tools/cosmovisor/cmd/cosmovisor/init.go (2 hunks)
- tools/cosmovisor/cmd/cosmovisor/init_test.go (14 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/run.go (2 hunks)
- tools/cosmovisor/process_test.go (18 hunks)
- tools/cosmovisor/testdata/download/cosmovisor/genesis/bin/autod (1 hunks)
- tools/cosmovisor/testdata/repo/chain2-zip_bin/autod (1 hunks)
- tools/cosmovisor/upgrade_test.go (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- tools/cosmovisor/args.go
- tools/cosmovisor/cmd/cosmovisor/init.go
- tools/cosmovisor/cmd/cosmovisor/init_test.go
- tools/cosmovisor/cmd/cosmovisor/root.go
- tools/cosmovisor/cmd/cosmovisor/run.go
🧰 Additional context used
📓 Path-based instructions (2)
tools/cosmovisor/process_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/upgrade_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (18)
tools/cosmovisor/upgrade_test.go (9)
1-1: Expanded platform support in build constraintThe update to include both
darwinandlinuxin the build constraint is a positive change. This expansion ensures that the tests can run on both Linux and macOS platforms, improving cross-platform compatibility and test coverage.
31-37: Improved test setup and path comparison inTestCurrentBinThe changes in
TestCurrentBinare well-implemented:
- The use of
prepareConfigenhances code reusability and readability.- Resolving symbolic links with
filepath.EvalSymlinksbefore comparison ensures more accurate path validations.These improvements make the tests more robust and reliable.
Also applies to: 42-45, 54-57, 69-72
106-106: UpdatedassertCurrentLinkmethod signatureThe change to accept a pointer to
cosmovisor.Configin theassertCurrentLinkmethod is a good improvement. This modification:
- Aligns with the changes observed in the test functions.
- Can improve performance by avoiding unnecessary copying of the
Configstruct.- Allows for potential modifications to the
Configobject if needed in future implementations.This change enhances consistency and efficiency in the codebase.
Also applies to: 116-116
121-129: Consistent improvements inTestUpgradeBinaryNoDownloadUrlThe changes in
TestUpgradeBinaryNoDownloadUrlmaintain consistency with the improvements seen in other test functions:
- The use of
prepareConfigsimplifies the test setup.- The application of
filepath.EvalSymlinksensures accurate path comparisons.These consistent improvements across the test suite enhance overall code quality and test reliability.
Also applies to: 135-138, 147-151, 165-168
244-252: Enhanced cross-platform support inTestOsArchThe updates to
TestOsArchare excellent improvements:
- The test now checks for both Linux and Darwin (macOS) architectures, aligning with the updated build constraint.
- The use of
slices.Containssimplifies the code and improves readability.These changes ensure better cross-platform compatibility testing and make the code more maintainable.
258-258: Improved flexibility incopyTestDatafunctionThe update to the
copyTestDatafunction signature is a positive change:
- Accepting a string for the test data path allows for more flexible usage of the function.
- This modification makes the function more generic and potentially reusable across different test scenarios.
The change enhances the function's utility while maintaining its core functionality.
Also applies to: 261-261
77-83: Consistent improvements inTestCurrentAlwaysSymlinkToDirectoryThe changes in this test function align well with the improvements made in
TestCurrentBin. The use ofprepareConfigandfilepath.EvalSymlinksenhances the test's reliability.Note that
assertCurrentLinknow receives a pointer tocosmovisor.Config. This change should be verified to ensure it's consistent with the method's implementation and doesn't introduce any unintended side effects.To verify the
assertCurrentLinkmethod signature change, please run:#!/bin/bash # Check the signature of assertCurrentLink method rg --type go "func \(.*\) assertCurrentLink\(.*\)" tools/cosmovisor/Also applies to: 87-91, 98-102, 106-106
182-182: Updated test cases and improved setup inTestUpgradeBinaryThe changes in
TestUpgradeBinaryare well-implemented:
- The test cases have been updated with new file paths, likely reflecting changes in the project structure or test data organization.
- The introduction of
prepareConfigfor test setup aligns with the improvements seen in other tests, enhancing consistency and readability.These changes improve the test's maintainability and alignment with the project's current structure.
To ensure the new file paths are correct, please run:
#!/bin/bash # Check if the new file paths exist for path in "testdata/repo/raw_binary/autod" "testdata/repo/chain3-zip_dir/autod.zip" "testdata/repo/bad_dir/autod"; do if [ -e "$path" ]; then echo "Path exists: $path" else echo "Path does not exist: $path" fi doneAlso applies to: 187-187, 191-191, 196-196, 200-200, 213-220
9-9: Added import forslicespackageThe addition of the
slicespackage import is a good improvement. This package provides efficient and readable slice operations. However, ensure that the project's minimum Go version is set to 1.21 or higher, as theslicespackage was introduced in Go 1.21.To verify the Go version, please run the following command:
✅ Verification successful
Confirmed Go Version
The project's Go version is 1.23.1, which satisfies the minimum requirement for the
slicespackage. The addition of theslicesimport is approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the Go version in go.mod file grep -E "^go [0-9]+\.[0-9]+" go.modLength of output: 839
Script:
#!/bin/bash # Extract the Go version specified in go.mod grep ^go go.mod | awk '{print $2}'Length of output: 45
tools/cosmovisor/process_test.go (9)
20-20: Import addition looks goodThe addition of the cosmovisor package import is consistent with the refactoring mentioned in the summary. This change allows direct use of the cosmovisor package in the test file.
96-105: Consistent improvements in TestPlanDisableRecaseThe changes in this test function are consistent with the improvements made in TestLaunchProcess. They enhance the test's robustness and maintainability by:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.These changes are approved as they maintain consistency across the test suite.
Also applies to: 115-118, 136-138, 150-152
157-166: Consistent improvements in TestLaunchProcessWithRestartDelayThe changes in this test function align with the improvements made in previous test functions. They enhance the test's robustness and maintainability by:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.- Using
workDirfor portable test data paths.These changes are approved as they maintain consistency across the test suite.
Also applies to: 176-178
200-209: Consistent improvements in TestPlanShutdownGraceThe changes in this test function are in line with the improvements made in previous test functions. They enhance the test's robustness and maintainability by:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.- Using
workDirfor portable test data paths.These changes are approved as they maintain consistency across the test suite.
Also applies to: 218-220, 238-240, 252-254
264-273: Consistent improvements in TestLaunchProcessWithDownloadsThe changes in this test function align with the improvements made in previous test functions. They enhance the test's robustness and maintainability by:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.- Using
workDirfor portable test data paths.These changes are approved as they maintain consistency across the test suite.
Also applies to: 281-283, 299-301, 316-318, 333-335
345-355: Consistent improvements and new test scenario in TestLaunchProcessWithDownloadsAndMissingPreupgradeThe changes in this test function are consistent with the improvements in previous functions and introduce a new test scenario:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.- Using
workDirfor portable test data paths.- Adding
CustomPreUpgrade: "missing.sh"to test a scenario with a missing pre-upgrade script.These changes are approved as they maintain consistency across the test suite and expand the test coverage.
Also applies to: 364-366
387-397: Comprehensive improvements and new test scenario in TestLaunchProcessWithDownloadsAndPreupgradeThe changes in this test function align with previous improvements and introduce a new test scenario:
- Using
prepareConfigfor centralized configuration setup.- Employing
filepath.EvalSymlinksfor accurate binary path comparisons.- Using
workDirfor portable test data paths.- Adding
CustomPreUpgrade: "preupgrade.sh"to test a scenario with a custom pre-upgrade script.- New assertions to verify the creation of upgrade-related files.
These changes are approved as they maintain consistency, expand test coverage, and verify the correct execution of custom pre-upgrade scripts.
Also applies to: 406-408, 424-426, 429-429, 444-446, 449-449, 464-466
Line range hint
470-508: No changes in TestSkipUpgradeThe TestSkipUpgrade function remains unchanged. This is appropriate if its functionality is still valid and doesn't require updates based on the recent refactoring.
Line range hint
511-557: No changes in TestUpgradeSkipHeightsThe TestUpgradeSkipHeights function remains unchanged. This is appropriate if its functionality is still valid and doesn't require updates based on the recent refactoring.
tools/cosmovisor/testdata/download/cosmovisor/genesis/bin/autod
Outdated
Show resolved
Hide resolved
troian
force-pushed
the
cosmovisor-relative-symlink
branch
from
ca6739c
to
4c1b2ef
Compare
That would be great, thanks! So that we can see green in this PR |
|
@julienrbrt done #22102 |
troian
force-pushed
the
cosmovisor-relative-symlink
branch
from
4c1b2ef
to
52e17b7
Compare
troian
force-pushed
the
cosmovisor-relative-symlink
branch
from
52e17b7
to
79cb90f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tools/cosmovisor/upgrade_test.go (1)
266-277: LGTM: IntroducedprepareConfigfor consistent test setupThe addition of the
prepareConfigfunction is an excellent improvement to the test suite:
- It enhances test setup consistency across different test cases.
- It simplifies the test code by encapsulating common setup logic.
- The use of a temporary directory ensures proper test isolation.
This change contributes to the overall maintainability and reliability of the test suite.
Consider adding a defer statement to change the working directory back to the original one after the test completes. This ensures that the working directory is reset even if the test panics:
func prepareConfig(t *testing.T, testData string, config cosmovisor.Config) *cosmovisor.Config { t.Helper() tmpdir := copyTestData(t, testData) config.Home = tmpdir + originalDir, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { + _ = os.Chdir(originalDir) + }) err := os.Chdir(config.Root()) require.NoError(t, err) return &config }tools/cosmovisor/process_test.go (1)
32-53: LGTM: Improved test setup and file path handlingThe changes in the
TestLaunchProcessfunction enhance the test setup by using a centralizedprepareConfigfunction and improve file path handling withfilepath.EvalSymlinks. These modifications increase the reliability and maintainability of the test.Consider adding a brief comment explaining the purpose of using
filepath.EvalSymlinksto help future maintainers understand the importance of this change.Also applies to: 71-74, 86-89
🛑 Comments failed to post (1)
tools/cosmovisor/cmd/cosmovisor/init_test.go (1)
734-743:
⚠️ Potential issueAvoid changing the working directory in tests
The
prepareConfigfunction changes the current working directory toconfig.Homeusingos.Chdir, which can lead to unintended side effects in tests, especially when running tests in parallel. It's recommended to avoid changing the global working directory and instead use absolute paths or pass the directory as a parameter where needed.
Signed-off-by: Artur Troian <[email protected]>
troian
force-pushed
the
cosmovisor-relative-symlink
branch
from
79cb90f
to
f4bb7c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
tools/cosmovisor/CHANGELOG.md (2)
Line range hint
48-50: LGTM with a minor suggestionThe new feature is well-described and properly linked to the GitHub pull request. The multi-line format is appropriate for the detailed description.
Consider a minor rewording for clarity:
- * [#21932](https://github.com/cosmos/cosmos-sdk/pull/21932) Add `cosmovisor show-upgrade-info` command to display the upgrade-info.json into stdout. + * [#21932](https://github.com/cosmos/cosmos-sdk/pull/21932) Add `cosmovisor show-upgrade-info` command to display the contents of upgrade-info.json to stdout.This small change makes the purpose of the command slightly clearer.
🧰 Tools
🪛 Markdownlint
48-48: null
Multiple headings with the same content(MD024, no-duplicate-heading)
Line range hint
1-50: Overall, excellent changelog managementThe CHANGELOG.md file is well-structured and follows best practices for changelog maintenance. It effectively categorizes changes, links to relevant GitHub pull requests, and provides clear descriptions of new features, improvements, and bug fixes across multiple versions.
For consistency, consider adding the release date for the "Unreleased" section once it's published, following the format used in previous releases (e.g.,
## [v1.7.0] - YYYY-MM-DD).Great job on maintaining a comprehensive and user-friendly changelog!
🧰 Tools
🪛 Markdownlint
48-48: null
Multiple headings with the same content(MD024, no-duplicate-heading)
tools/cosmovisor/process_test.go (4)
22-26: LGTM with a minor suggestion for improvementThe introduction of the
workDirvariable and theinit()function is a good improvement for flexible path handling in the test cases. However, I suggest handling the potential error fromos.Getwd().Consider updating the
init()function to handle the potential error:func init() { - workDir, _ = os.Getwd() + var err error + workDir, err = os.Getwd() + if err != nil { + panic(fmt.Sprintf("failed to get working directory: %v", err)) + } }This change ensures that any issues with getting the working directory are immediately apparent.
95-104: LGTM with a suggestion for clarityThe use of
prepareConfigis consistent with earlier changes, which is good. The addition of theDisableRecaseparameter is likely necessary for this specific test case.Consider adding a brief comment explaining the purpose of the
DisableRecaseparameter:cfg := prepareConfig( t, fmt.Sprintf("%s/%s", workDir, "testdata/norecase"), cosmovisor.Config{ Name: "dummyd", PollInterval: 20, UnsafeSkipBackup: true, + // DisableRecase is set to true to test behavior when plan name recasing is disabled DisableRecase: true, }, )This will help other developers understand the purpose of this parameter in the test setup.
156-165: LGTM with a suggestion for clarityThe use of
prepareConfigis consistent with earlier changes, which is good. The addition of theRestartDelayparameter is likely necessary for testing the system's behavior with a delay between restarts.Consider adding a brief comment explaining the purpose of the
RestartDelayparameter:cfg := prepareConfig( t, fmt.Sprintf("%s/%s", workDir, "testdata/validate"), cosmovisor.Config{ Name: "dummyd", + // RestartDelay is set to test the system's behavior with a delay between restarts RestartDelay: 5 * time.Second, PollInterval: 20, UnsafeSkipBackup: true, }, )This will help other developers understand the purpose of this parameter in the test setup.
199-208: LGTM with a suggestion for clarityThe use of
prepareConfigis consistent with earlier changes, which is good. The addition of theShutdownGraceparameter is likely necessary for testing the system's graceful shutdown behavior.Consider adding a brief comment explaining the purpose of the
ShutdownGraceparameter:cfg := prepareConfig( t, fmt.Sprintf("%s/%s", workDir, "testdata/dontdie"), cosmovisor.Config{ Name: "dummyd", PollInterval: 15, UnsafeSkipBackup: true, + // ShutdownGrace is set to test the system's graceful shutdown behavior ShutdownGrace: 2 * time.Second, }, )This will help other developers understand the purpose of this parameter in the test setup.
tools/cosmovisor/args.go (2)
116-117: Improve comment formatting to match Go style guidelinesThe comments should be full sentences starting with a capital letter and ending with a period, following the Go coding conventions outlined in the Uber Go Style Guide.
Apply this diff to correct the comments:
- // workdir is set to cosmovisor directory so relative - // symlinks are getting resolved correctly + // Workdir is set to the cosmovisor directory so relative + // symlinks are resolved correctly.
142-143: Improve comment formatting to match Go style guidelinesThe comments should start with a capital letter and end with a period to adhere to the standard Go commenting practices.
Apply this diff to update the comments:
- // if nothing here, fallback to genesis - // if it is there, ensure it is a symlink + // If nothing is here, fall back to genesis. + // If it is there, ensure it is a symlink.tools/cosmovisor/args_test.go (2)
Line range hint
457-468: Consider refactoringnewConfigto reduce parameter list lengthThe
newConfigfunction now has a significant number of parameters, which can reduce code readability and maintainability. According to the Uber Go Style Guide, functions with many parameters should be refactored to accept a struct or use the functional options pattern to improve clarity.
Line range hint
523-768: Add unit tests forGRPCAddressandShutdownGraceconfiguration variationsThe addition of
GRPCAddressandShutdownGraceparameters enhances configurability, but the current tests primarily use default or fixed values. Consider adding unit tests that cover different scenarios, including custom values and invalid inputs, to ensure robust handling and proper error reporting for these new configuration options.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 52e17b7247230dd378e0a992cd27f57b2a45917a and f4bb7c1.
📒 Files selected for processing (9)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (3 hunks)
- tools/cosmovisor/args_test.go (18 hunks)
- tools/cosmovisor/cmd/cosmovisor/init.go (2 hunks)
- tools/cosmovisor/cmd/cosmovisor/init_test.go (14 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/run.go (2 hunks)
- tools/cosmovisor/process_test.go (18 hunks)
- tools/cosmovisor/upgrade_test.go (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/cosmovisor/cmd/cosmovisor/init.go
- tools/cosmovisor/cmd/cosmovisor/root.go
- tools/cosmovisor/cmd/cosmovisor/run.go
- tools/cosmovisor/upgrade_test.go
🧰 Additional context used
📓 Path-based instructions (5)
tools/cosmovisor/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/init_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/process_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (24)
tools/cosmovisor/CHANGELOG.md (2)
45-45: LGTM: Well-formatted changelog entryThe new improvement is clearly described and properly linked to the corresponding GitHub pull request. This entry follows the established changelog format.
46-46: LGTM: Properly formatted changelog entryThis improvement is clearly described and correctly linked to its GitHub pull request. The entry adheres to the established changelog format.
tools/cosmovisor/process_test.go (6)
32-40: LGTM: Improved configuration setupThe use of the
prepareConfigfunction to initialize thecfgvariable is a good improvement. It centralizes the configuration setup, making the tests more maintainable and flexible.The use of
workDirin the path and the configuration struct for parameter passing are particularly good practices.
50-53: LGTM: Improved symlink handlingThe use of
filepath.EvalSymlinksto resolve symbolic links before comparing paths is a good improvement. This change enhances the test's ability to catch potential issues with symlink handling in the actual implementation.This modification makes the test more robust and accurate.
71-74: LGTM: Consistent symlink handling after upgradeThe use of
filepath.EvalSymlinkshere maintains consistency with the earlier symlink resolution approach. This ensures that the test accurately verifies the upgrade process's handling of symlinks.This change contributes to the overall robustness of the test suite.
86-89: LGTM: Thorough symlink verificationThe consistent use of
filepath.EvalSymlinkshere completes the symlink verification process throughout the upgrade cycle. This ensures that the final state after all upgrades is correctly verified.This change contributes to a comprehensive test of the upgrade process's symlink handling.
114-117: LGTM: Consistent symlink handling across test casesThe consistent use of
filepath.EvalSymlinksacross these different test cases and stages of the upgrade process is commendable. This approach ensures thorough and accurate verification of symlink handling throughout the entire upgrade cycle.This consistency contributes to the robustness and reliability of the test suite.
Also applies to: 135-137, 149-151
217-219: LGTM: Comprehensive and consistent symlink handlingThe consistent use of
filepath.EvalSymlinksacross all these test cases and stages of the upgrade process is excellent. This uniform approach ensures thorough and accurate verification of symlink handling throughout all scenarios tested in this file.This consistency significantly enhances the robustness and reliability of the entire test suite, providing comprehensive coverage of symlink-related behaviors.
Also applies to: 237-239, 251-253, 280-282, 297-300, 315-317, 332-334, 362-365, 405-407, 422-425, 443-445, 463-465
tools/cosmovisor/cmd/cosmovisor/init_test.go (14)
141-142: Interface implementation check looks good.The explicit interface checks for
io.Readerandio.Writerensure thatBufferedPipecorrectly implements these interfaces. This is a good practice for maintaining type safety and documenting the intended behavior of the struct.
170-171: Function signature update improves memory efficiency.Changing the return type of
NewBufferedPipefromBufferedPipeto*BufferedPipeis a good optimization. It avoids unnecessary copying of the struct and allows for more efficient memory usage, especially if theBufferedPipestruct grows larger in the future.
187-187: Function signature update for consistency.The return type change for
StartNewBufferedPipeto*BufferedPipemaintains consistency with theNewBufferedPipefunction. This is a good practice for maintaining a coherent API.
217-217: Improved error handling in buffering goroutine.The addition of a blank line after the error handling in the buffering goroutine and before closing the writer in the
Collectmethod improves readability. It's a minor change but helps in visually separating different logical blocks of code.Also applies to: 242-242
252-253: Method receiver types updated for consistency.Changing the receiver types for
ReadandWritemethods from value receivers to pointer receivers is a good practice. It ensures consistency with other methods ofBufferedPipeand allows for potential future modifications to the struct within these methods.Also applies to: 257-258
279-279: Simplified return statement in NewCapturingLogger.The change to directly return
bufferedStdOutinstead of taking its address simplifies the code without changing its functionality. This is a good refactoring that improves code readability.
454-454: Error message update for clarity.The error message has been updated to be more specific about the nature of the error. This change improves the clarity of the error message, making it easier for developers to understand and debug issues.
487-490: New method prepareConfig introduced.The addition of the
prepareConfigmethod is a good refactoring. It centralizes the setup of the test environment, making the test cases more readable and maintainable. The use of a temporary directory for each test ensures isolation between tests.Also applies to: 492-506
493-500: Test case refactored for better readability.The refactoring of this test case improves its readability and maintainability. The use of the new
prepareConfigmethod simplifies the setup, and the additional checks for the existence and permissions of the created files enhance the test's robustness.Also applies to: 518-523
509-514: Updated log expectations in test cases.The expected log messages have been updated to reflect the changes in the implementation. This ensures that the tests accurately verify the behavior of the
InitializeCosmovisorfunction. The addition of the config file creation log message is particularly important as it verifies a new feature.Also applies to: 569-569, 613-613
543-554: Improved test setup and verification.The addition of
filepath.EvalSymlinksto resolve the actual path of the genesis binary directory and executable improves the robustness of the test. It ensures that the test works correctly even if the filesystem uses symlinks, which is a good practice for writing more reliable tests.
596-606: Test case setup improved.The use of
s.setEnvand the newprepareConfigmethod improves the consistency and readability of the test setup. The addition of symlink resolution enhances the test's ability to verify the correct behavior of theInitializeCosmovisorfunction.
716-718: Improved file handling with defer.The addition of a deferred file close operation is a good practice. It ensures that the file is properly closed even if an error occurs during the reading process, preventing potential resource leaks.
733-743: New prepareConfig method implementation.The
prepareConfigmethod is a well-implemented helper function for test setup. It creates a temporary directory for each test, sets it as the current working directory, and returns the updated configuration. This approach ensures test isolation and simplifies the setup process in individual test cases.tools/cosmovisor/args.go (1)
391-406: LGTM!The adjustments to the symlink handling correctly implement relative symlinks, enhancing functionality in Docker environments with mounted volumes. The code changes are accurate and align with best practices.
tools/cosmovisor/args_test.go (1)
Line range hint
799-808: Ensure correct parsing and error handling forShutdownGracedurationIn
setupConfig,shutdownGraceis set to10000000000, representing 10 seconds in nanoseconds. Please verify that the parsing ofShutdownGracefrom the environment variable handles duration strings correctly (e.g.,"10s","500ms") and that invalid inputs result in appropriate errors. Adding tests for various duration formats and invalid values can improve the robustness of the configuration parsing.
|
@julienrbrt any ETA on new cosmovisor release with this PR? |
Yes, probably at the same time as the next 0.50 patch release. We check monthly what to release. |
if cosmovisor used within docker
daemon workdir can be mounted as volume which leads to current symlink no working on the host machine
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
prepare-upgradecommand for easier upgrade preparation.cosmovisor show-upgrade-infocommand to display upgrade information.--cosmovisor-configflag for specifying configuration file paths.Improvements
Bug Fixes
add-upgradecommand.