WIP feat(tools/cosmovisor): create current symlink as relative #21891
Conversation
troian
requested review from
julienrbrt,
facundomedica and
a team
as code owners
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested labels
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
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
| @@ -14,7 +14,7 @@ func NewRootCmd() *cobra.Command { | |||
| } | |||
|
|
|||
| rootCmd.AddCommand( | |||
| NewIntCmd(), | |||
| NewInitCmd(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation updates required.
Please update the following documentation files to reflect the change from NewIntCmd() to NewInitCmd():
client/v2/CHANGELOG.mdclient/v2/README.mddocs/architecture/adr-058-auto-generated-cli.mddocs/learn/advanced/07-cli.mdtools/cosmovisor/CHANGELOG.mdtools/cosmovisor/README.mdtools/cosmovisor/RELEASE_NOTES.md
Analysis chain
LGTM. Verify related updates and update changelog.
The change from NewIntCmd() to NewInitCmd() aligns with the PR objectives and appears to be a logical modification to the CLI structure. This change likely introduces an initialization command, which is consistent with the goal of creating a relative symlink for the current directory.
To ensure completeness, please:
- Verify that related documentation (e.g., CLI help texts, user guides) has been updated to reflect this change.
- Confirm that appropriate tests have been added or updated for the new
InitCmd. - Update the changelog to mention this CLI command change.
Run the following script to check for related files that might need updates:
This will help identify files that might need to be updated due to the command change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find files that might need updates due to the command change
# Search for files mentioning the old command
echo "Files potentially referencing the old 'int' command:"
rg --type go --type md "NewIntCmd|int\s+command" -l
# Search for test files related to commands
echo "Test files related to commands:"
fd -e go test_.*cmd
# Search for documentation files
echo "Documentation files that might need updates:"
fd -e md -e txt | rg "cli|command|cosmovisor"
Length of output: 630
tools/cosmovisor/args.go
Outdated
| // workdir is set to cosmovisor directory so relative | ||
| // symlinks are getting resolved correctly | ||
| if err := os.Symlink(genesisDir, currentLink); err != nil { | ||
| return "", err | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Working directory is not set to the cosmovisor directory
The shell script indicates that the current working directory is /home/jailuser/git instead of the expected cosmovisor directory. This misconfiguration can lead to incorrect resolution of relative symlinks as described.
Analysis chain
Approve changes with a suggestion for additional safeguards
The simplification of the symlink creation process improves code readability. However, the reliance on the correct working directory setting could potentially lead to issues.
Consider adding a check to ensure the working directory is set correctly before creating the symlink. This could help prevent potential errors if the method is called from an unexpected context.
To verify the current working directory setting, you can run the following script:
This script will help ensure that the working directory assumption is valid.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the current working directory setting
# Test: Check if the current working directory is set to the cosmovisor directory
pwd
ls -la
Length of output: 4476
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 <troian.ap@gmail.com>
if cosmovisor used within docker daemon workdir can be mounted as volume which leads to current symlink no working on the host machine Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian
force-pushed
the
cosmovisor-relative-symlink
branch
from
bb72f63
to
f932209
Compare
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
currentsymlink in the cosmovisor module.[]bytefor addresses.