Skip to content
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

feat(tools/cosmovisor): Introduce a new optional config variable for setting custom path to application data directory #21971

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

0x4r45h
Copy link

@0x4r45h 0x4r45h commented Sep 29, 2024

Description

Closes: #20947

Introduces the configuration DataPath to allow users to set an arbitrary location for the application data directory. This option can be set through the environment variable DAEMON_DATA_DIR or via the configuration TOML file using daemon_data_dir.

These changes are designed to be backward compatible, meaning they won't affect existing users unless they explicitly set an absolute path for this configuration.


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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a new environment variable EnvDataPath for enhanced configuration.
    • Added a DataPath option in the configuration for flexible data directory management.
    • New shell scripts added to support upgrade notifications and logging.
    • Added support for a custom path to the application data directory.
    • Introduced the cosmovisor show-upgrade-info command to display upgrade information.
  • Bug Fixes

    • Improved validation for the DataPath to ensure it is set correctly and points to a valid directory.
  • Tests

    • Updated existing tests and added new cases to validate the DataPath functionality.
  • Documentation

    • Updated README.md to include the new DAEMON_DATA_DIR configuration option.
    • Updated CHANGELOG.md to reflect recent improvements and features.

Copy link
Contributor

coderabbitai bot commented Sep 29, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes introduce a configurable data directory path in the Cosmovisor tool, allowing users to specify a custom path for application data instead of relying on a hardcoded directory. This includes the addition of a new environment variable, EnvDataPath, and updates to various methods and tests to accommodate the new configuration option. The changes enhance validation logic and ensure that the new data path is properly integrated into the existing functionality.

Changes

Files Change Summary
tools/cosmovisor/args.go, tools/cosmovisor/args_test.go Introduced EnvDataPath and DataPath in Config struct; added validation for DataPath; updated methods to use DataPath instead of hardcoded values; modified tests to include new configuration and validation cases.
tools/cosmovisor/cmd/cosmovisor/init.go Minor formatting change in minConfigValidate function (removed a blank line).
tools/cosmovisor/process.go, tools/cosmovisor/process_test.go Updated doBackup method to use DataPath for backup operations; modified multiple test functions to include DataPath in configurations and added a new test for custom data location.
tools/cosmovisor/testdata/custom-data-path/cosmovisor/genesis/bin/dummyd, tools/cosmovisor/testdata/custom-data-path/cosmovisor/upgrades/chain2/bin/dummyd Added new shell script files for testing purposes, implementing basic logging functionality.

Assessment against linked issues

Objective Addressed Explanation
Allow users to specify an absolute path to the application data directory (#[20947])
Decouple Cosmovisor from the application home directory for flexibility (#[20947])

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the C:Cosmovisor Issues and PR related to Cosmovisor label Sep 29, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (11)
tools/cosmovisor/testdata/custom-data-path/cosmovisor/upgrades/chain2/bin/dummyd (3)

3-4: Echo statements provide clear output

The echo statements effectively communicate the script's progress and display passed arguments. This is useful for debugging and demonstration purposes.

Consider enclosing the $@ in double quotes to handle arguments with spaces correctly:

-echo Args: $@
+echo "Args: $@"

Also applies to: 6-6


5-5: Sleep command simulates processing time

The sleep 1 command introduces a 1-second delay, which can be useful for simulating processing time or ensuring outputs are visible during testing.

Consider adding a comment to explain the purpose of the sleep command:

+# Simulate processing time
sleep 1

1-6: Well-structured mock daemon script for testing

This script effectively simulates a simple daemon process, which is appropriate for testing Cosmovisor's upgrade functionality. The structure is clear and concise, making it easy to understand and maintain.

Consider adding a comment at the beginning of the file to explicitly state its purpose:

#!/bin/sh

# Mock daemon script for testing Cosmovisor upgrades
# Simulates 'chain 2' daemon process

This would help other developers quickly understand the script's role in the testing process.

tools/cosmovisor/testdata/custom-data-path/cosmovisor/genesis/bin/dummyd (3)

4-5: LGTM: Argument check is correct, but consider adding a comment.

The argument check and exit condition are implemented correctly. However, to improve readability and maintainability, consider adding a comment explaining the purpose of the 4th argument and the significance of the 1001 exit code.

Consider adding a comment like this:

# Exit with code 1001 if the 4th argument (upgrade info file path) is not provided

6-7: LGTM: Upgrade simulation looks good, but consider parameterization.

The upgrade simulation is correctly implemented, creating both a console output and a JSON file with the upgrade information.

For improved flexibility in testing scenarios, consider parameterizing the upgrade name, height, and info. This could be done by using additional command-line arguments or environment variables. For example:

UPGRADE_NAME=${UPGRADE_NAME:-"chain2"}
UPGRADE_HEIGHT=${UPGRADE_HEIGHT:-49}
echo "UPGRADE \"$UPGRADE_NAME\" NEEDED at height: $UPGRADE_HEIGHT: {}"
echo "{\"name\":\"$UPGRADE_NAME\",\"height\":$UPGRADE_HEIGHT,\"info\":\"\"}" > $4

8-9: LGTM: Final part simulates unreachable code, but needs a comment.

The final sleep and echo statements are likely intended to simulate unreachable code for testing purposes.

To improve clarity, consider adding a comment explaining the purpose of this "unreachable" code. For example:

# Simulate unreachable code for testing error handling
sleep 2
echo Never should be printed!!!
tools/cosmovisor/process.go (1)

190-191: LGTM! Consider enhancing error handling.

The change from filepath.Join(l.cfg.Home, "data") to l.cfg.DataPath aligns well with the PR objective of introducing a configurable data directory path. This modification enhances flexibility for users, particularly in scenarios like Docker deployments.

Consider adding a check to ensure l.cfg.DataPath is not empty before proceeding with the backup. This could prevent potential issues if the configuration is not set correctly:

if l.cfg.DataPath == "" {
    return fmt.Errorf("data path is not set")
}

This addition would improve error handling and provide clearer feedback if the configuration is missing.

tools/cosmovisor/process_test.go (1)

374-419: LGTM: Well-implemented test for custom data location

The new TestPlanCustomDataLocation function is a valuable addition that verifies the correct behavior of Cosmovisor when using a custom data directory. The test structure is consistent with other test functions, and it effectively checks the upgrade process with a non-default data path.

One minor suggestion for improvement:

Consider adding an assertion to verify that the custom data path is actually being used. For example, you could check if the upgrade info file is created in the custom location:

customUpgradeInfoPath := filepath.Join(dataPath, "upgrade-info.json")
require.FileExists(t, customUpgradeInfoPath)

This additional check would provide stronger verification that the custom data path is respected throughout the upgrade process.

tools/cosmovisor/args.go (2)

220-231: LGTM: DataPath initialization added with fallback to default.

The DataPath is correctly initialized from the environment variable with a fallback to a default value, ensuring backward compatibility.

Consider adding a comment explaining the fallback behavior for clarity:

 if cfg.DataPath == "" {
+	// Fallback to default data directory if not specified
 	cfg.DataPath = cfg.DefaultDataDirPath()
 }

359-372: LGTM: Comprehensive validation for DataPath added.

The validation logic for DataPath is thorough and consistent with other validations. It ensures that the path is set, absolute, and points to a valid directory.

For consistency with other validations, consider using cfg.DataPath instead of EnvDataPath in the error message:

-		errs = append(errs, fmt.Errorf("%s is not set", EnvDataPath))
+		errs = append(errs, fmt.Errorf("DataPath is not set"))
tools/cosmovisor/args_test.go (1)

286-287: Use s.T().Logf instead of fmt.Println for test logging.

It's recommended to use s.T().Logf for logging within tests instead of fmt.Println, as it integrates better with the testing framework's output handling.

Apply this change:

-	fmt.Println(title)
+	s.T().Logf("%s", title)
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d5f24de and e4461b4.

📒 Files selected for processing (7)
  • tools/cosmovisor/args.go (8 hunks)
  • tools/cosmovisor/args_test.go (18 hunks)
  • tools/cosmovisor/cmd/cosmovisor/init.go (0 hunks)
  • tools/cosmovisor/process.go (1 hunks)
  • tools/cosmovisor/process_test.go (8 hunks)
  • tools/cosmovisor/testdata/custom-data-path/cosmovisor/genesis/bin/dummyd (1 hunks)
  • tools/cosmovisor/testdata/custom-data-path/cosmovisor/upgrades/chain2/bin/dummyd (1 hunks)
💤 Files with no reviewable changes (1)
  • tools/cosmovisor/cmd/cosmovisor/init.go
🧰 Additional context used
📓 Path-based instructions (4)
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/process.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

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 (34)
tools/cosmovisor/testdata/custom-data-path/cosmovisor/upgrades/chain2/bin/dummyd (1)

1-1: Appropriate shebang for shell script

The use of #!/bin/sh is a good choice for maximum compatibility across Unix-like systems.

tools/cosmovisor/testdata/custom-data-path/cosmovisor/genesis/bin/dummyd (1)

1-3: LGTM: Shebang and initial output look good.

The shebang is correct for a shell script, and the initial echo statement appropriately simulates a genesis process output by printing "Genesis" followed by all passed arguments.

tools/cosmovisor/process_test.go (7)

28-29: LGTM: Correct implementation of the new DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the PR objective. The dataPath is correctly set as a subdirectory of the home directory, which aligns with the expected behavior.


74-75: LGTM: Consistent implementation of DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the previous test function. The dataPath is correctly set as a subdirectory of the home directory, maintaining consistency across test cases.


119-120: LGTM: Consistent implementation of DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the previous test functions. The dataPath is correctly set as a subdirectory of the home directory, maintaining consistency across test cases.


151-152: LGTM: Consistent implementation of DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the previous test functions. The dataPath is correctly set as a subdirectory of the home directory, maintaining consistency across test cases.


201-202: LGTM: Consistent implementation of DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the previous test functions. The dataPath is correctly set as a subdirectory of the home directory, maintaining consistency across test cases.


265-269: LGTM: Consistent implementation of DataPath config

The addition of the DataPath field to the cosmovisor.Config structure is consistent with the previous test functions. The dataPath is correctly set as a subdirectory of the home directory in both TestLaunchProcessWithDownloadsAndMissingPreupgrade and TestLaunchProcessWithDownloadsAndPreupgrade functions, maintaining consistency across test cases.

Also applies to: 303-307


Line range hint 1-529: Summary: Comprehensive and consistent implementation of DataPath config

The changes in this file consistently implement the new DataPath configuration across all relevant test functions. The addition of the TestPlanCustomDataLocation function provides good coverage for the new feature, ensuring that Cosmovisor correctly handles custom data directory paths.

The modifications align well with the PR objective of introducing a new optional config variable for setting a custom path to the application data directory. The test suite now comprehensively covers various scenarios, including the new custom data path functionality.

tools/cosmovisor/args.go (5)

34-34: LGTM: New environment variable constant added.

The new constant EnvDataPath follows the existing naming convention and has a clear, descriptive value.


67-67: LGTM: New DataPath field added to Config struct.

The DataPath field is correctly added with appropriate TOML and mapstructure tags, aligning with the PR objective.


115-117: LGTM: DefaultDataDirPath method added.

The DefaultDataDirPath method provides a clear and consistent way to get the default data directory path, aligning with the PR objectives.


112-114: LGTM: UpgradeInfoFilePath updated to use configurable DataPath.

The method now correctly uses the configurable DataPath instead of a hardcoded directory, implementing the new feature as intended.


571-571: LGTM: DataPath added to DetailString output.

The DataPath configuration is correctly added to both the configurable and derived values in the DetailString output, ensuring comprehensive configuration reporting.

Also applies to: 586-586

tools/cosmovisor/args_test.go (20)

36-36: Adding DataPath field to cosmovisorEnv struct.

The addition of the DataPath field to the cosmovisorEnv struct correctly accommodates the new configuration variable required for specifying a custom data directory in tests.


64-64: Including EnvDataPath in environment variable map with allowEmpty: true.

Adding EnvDataPath to the environment variable map with allowEmpty: true allows the DataPath to be optional, which is appropriate since it can be unset to use the default path.


96-97: Handling EnvDataPath in the Set method of cosmovisorEnv.

This correctly adds support for the EnvDataPath environment variable within the Set method, ensuring that the DataPath field is properly assigned during test setup.


236-238: Adding test case: "happy with absolute data path".

This test case verifies that the configuration is valid when DataPath is set to an absolute path, confirming that the validation logic correctly accepts absolute paths.


272-275: Adding test case: "missing data path".

This test case checks that the configuration is invalid when DataPath is missing or empty, ensuring that the DataPath is a required field and must be specified.


276-279: Adding test case: "no such data path dir".

This test verifies that the configuration validation fails when DataPath points to a non-existent directory, enforcing that the specified data path must exist.


280-283: Adding test case: "relative data path".

This test ensures that relative paths for DataPath are considered invalid, which correctly enforces the requirement for DataPath to be an absolute path.


302-302: Updating TestEnsureBin to include DataPath in configuration.

Including DataPath in the test configuration ensures that TestEnsureBin accurately reflects the updated Config structure and tests the new data path handling.


427-427: Extending TestDetailString to include DataPath.

Adding the dataPath variable and incorporating it into the configuration and expected output validates that DetailString correctly includes the DataPath information.

Also applies to: 438-438


452-452: Updating expected output in TestDetailString for DataPath.

Including EnvDataPath and Data Dir in the expected output confirms that DetailString reflects the DataPath configuration, ensuring accurate test verification.

Also applies to: 463-463


481-481: Modifying newConfig function to accept dataPath parameter.

This change allows tests to specify DataPath when creating configurations, enhancing test flexibility and ensuring the Config struct is properly constructed with the new field.

Also applies to: 499-499


517-517: Initializing dataDirPath in TestGetConfigFromEnv.

Defining dataDirPath provides a consistent absolute path for use in test configurations, which is important for testing DataPath handling.


795-799: Adding test case: "data dir bad (relative path)".

This test case checks that the configuration validation fails when DataPath is set to a relative path, ensuring that only absolute paths are accepted for DataPath.


800-805: Adding test case: "data dir good (abs path)".

This test verifies that an absolute DataPath is correctly accepted by the configuration validation, demonstrating proper handling of valid data paths.


806-809: Adding test case: "data dir good (empty path)".

This test ensures that when DataPath is not set or is empty, the default data path is used and the configuration remains valid, verifying default behavior.


844-844: Updating test setup to include DataPath.

Using newConfig with the dataPath parameter in the test setup ensures that all configurations used in tests include the DataPath, aligning with the updated Config structure.


874-874: Updating expected configuration in TestConfigFromFile to include DataPath.

Including DataPath in the expected configuration ensures that TestConfigFromFile accurately tests configurations loaded from a file with the new DataPath field.


889-889: Adjusting expected configuration when overridden by environment variables.

This confirms that when environment variables override the configuration, DataPath is still correctly handled in the expected configuration.


895-895: Including DataPath in expected configuration for empty config file scenario.

Ensuring DataPath is included when loading configuration from environment variables validates that the default DataPath is properly applied.


900-900: Setting environment variables including DataPath for tests.

This setup ensures that the environment accurately reflects all configuration parameters, including the new DataPath, allowing for comprehensive testing.

update README.md
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
tools/cosmovisor/README.md (2)

95-96: LGTM with a minor suggestion for clarity

The addition of the DAEMON_DATA_DIR configuration option is well-documented and aligns with the PR objectives. The explanation is clear and concise.

Consider adding a brief example of how to set this option, similar to other entries in this list. For instance:

- * `DAEMON_DATA_DIR` option to set an absolute path to the `data` directory. this path is used to detect upgrade info file and backups. If not set, `$DAEMON_HOME/data` is used.
+ * `DAEMON_DATA_DIR` option to set an absolute path to the `data` directory. This path is used to detect upgrade info file and backups. If not set, `$DAEMON_HOME/data` is used. For example: `export DAEMON_DATA_DIR=/path/to/data`.

This addition would provide users with a clear example of how to use this new configuration option.


131-132: LGTM with a minor grammatical suggestion

The addition of information about the DAEMON_DATA_DIR configuration option and its relationship to $DAEMON_HOME is valuable and well-integrated into the existing content. This change provides important clarity for users about the flexibility in setting up their directory structure.

There's a minor grammatical issue that could be improved. Consider the following change:

- Please note that `$DAEMON_HOME/cosmovisor` only stores the *application binaries*. The `cosmovisor` binary itself can be stored in any typical location (e.g. `/usr/local/bin`). The application will continue to store its data in the default data directory (e.g. `$HOME/.simapp`) or the data directory specified with the `--home` flag. `$DAEMON_HOME` is independent of the data directory and can be set to any location and the actual data directory path can be set through `DAEMON_DATA_DIR` config. If you set `$DAEMON_HOME` to the same directory as the data directory, you will end up with a configuration like the following:
+ Please note that `$DAEMON_HOME/cosmovisor` only stores the *application binaries*. The `cosmovisor` binary itself can be stored in any typical location (e.g. `/usr/local/bin`). The application will continue to store its data in the default data directory (e.g. `$HOME/.simapp`) or the data directory specified with the `--home` flag. `$DAEMON_HOME` is independent of the data directory and can be set to any location, and the actual data directory path can be set through the `DAEMON_DATA_DIR` config. If you set `$DAEMON_HOME` to the same directory as the data directory, you will end up with a configuration like the following:

This change improves the sentence structure and readability.

CHANGELOG.md (5)

Line range hint 1-51: Overall structure of the CHANGELOG is well-organized, but consider adding version numbers for unreleased changes

The CHANGELOG is well-structured, with clear sections for different types of changes. However, for the unreleased changes section, it would be helpful to include a tentative version number (e.g., "v0.47.0-alpha1" or similar) to give readers an idea of the upcoming version these changes will be part of.

Consider adding a tentative version number for the unreleased changes section.


Line range hint 7-10: Improve consistency in feature descriptions

The feature descriptions vary in their level of detail. Some are very brief, while others provide more context. For better readability and consistency, consider standardizing the format of feature descriptions.

Aim for a consistent level of detail in feature descriptions throughout the CHANGELOG.


Line range hint 14-18: Clarify the impact of API breaking changes

The section on API breaking changes is crucial for users upgrading their applications. Consider adding a brief explanation of the potential impact these changes might have on existing applications and any migration steps required.

Add a brief note about the potential impact of API breaking changes and provide links to migration guides if available.


Line range hint 20-24: Provide more context for state machine breaking changes

State machine breaking changes are significant and may require careful consideration during upgrades. It would be helpful to provide more context on why these changes were necessary and any precautions users should take when upgrading.

Expand on the reasons for state machine breaking changes and provide guidance for safe upgrades.


Line range hint 26-30: Consider grouping bug fixes by severity or impact

The bug fixes section lists various fixes without any apparent order. Consider grouping these fixes by severity (e.g., critical, major, minor) or by the component affected to help users quickly identify the most important fixes.

Group bug fixes by severity or affected component for easier navigation.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e4461b4 and 59dcf09.

📒 Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • tools/cosmovisor/README.md (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

tools/cosmovisor/README.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🔇 Additional comments (2)
tools/cosmovisor/README.md (1)

Line range hint 1-524: Overall assessment: Changes are well-implemented and documented

The introduction of the DAEMON_DATA_DIR configuration option has been well-documented in this README. The changes provide clear explanations of the new option's purpose and its relationship to existing configurations. The modifications are consistent with the rest of the document in terms of style and formatting.

The updates align well with the PR objectives and the AI-generated summary, offering users more flexibility in configuring their Cosmovisor setup. The documentation now clearly explains how users can separate the application binaries from the data directory, which is a valuable addition.

Only minor suggestions for improvement were made, primarily focusing on clarity and grammar. These suggestions, if implemented, would further enhance the quality of the documentation.

CHANGELOG.md (1)

Line range hint 32-51: Excellent detail in improvements section

The improvements section provides a good level of detail about the changes made. This is particularly helpful for users who want to understand the evolution of the SDK and take advantage of new optimizations or features.

The improvements section is well-detailed and informative.

@julienrbrt julienrbrt changed the title feat:(tools/cosmovisor): Introduce a new optional config variable for setting custom path to application data directory feat(tools/cosmovisor): Introduce a new optional config variable for setting custom path to application data directory Sep 30, 2024
CHANGELOG.md Outdated
@@ -48,6 +48,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input.
* (x/validate) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) New module solely responsible for providing ante/post handlers and tx validators for v2. It can be extended by the app developer to provide extra tx validators.
* In comparison to x/auth/tx/config, there is no app config to skip ante/post handlers, as overwriting them in baseapp or not injecting the x/validate module has the same effect.
* (cosmovisor) [#21971](https://github.com/cosmos/cosmos-sdk/pull/21971) Support Custom path to application data directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changelog should be under tools/cosmovisor/changelog

@@ -92,6 +92,7 @@ All arguments passed to `cosmovisor run` will be passed to the application binar

* `DAEMON_HOME` is the location where the `cosmovisor/` directory is kept that contains the genesis binary, the upgrade binaries, and any additional auxiliary files associated with each binary (e.g. `$HOME/.gaiad`, `$HOME/.regend`, `$HOME/.simd`, etc.).
* `DAEMON_NAME` is the name of the binary itself (e.g. `gaiad`, `regend`, `simd`, etc.).
* `DAEMON_DATA_DIR` option to set an absolute path to the `data` directory. this path is used to detect upgrade info file and backups. If not set, `$DAEMON_HOME/data` is used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the backup changes in this PR, but where are the data folder changes for the node?
How do you ensure the node saves its data to DAEMON_DATA_DIR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node data is written to the location specified by the underlying application's --home flag. Currently, the DAEMON_HOME for Cosmovisor must always be the same as the --home location. This PR decouples the two, allowing the Cosmovisor DAEMON_HOME to be set to any arbitrary location, as long as DAEMON_DATA_DIR points to the --home/data directory.
I've added an example to the README file.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
tools/cosmovisor/CHANGELOG.md (1)

Line range hint 42-46: LGTM! Consider adding more details to the feature description.

The new entries in the "Unreleased" section are well-formatted and follow the changelog guidelines. They accurately reflect the changes mentioned in the PR objectives.

For the feature entry, consider adding a brief explanation of why this new command is useful. For example:

- * [#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 upgrade-info.json into stdout, facilitating easier inspection and debugging of upgrade information.
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 59dcf09 and de6f983.

📒 Files selected for processing (1)
  • tools/cosmovisor/CHANGELOG.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
tools/cosmovisor/README.md (2)

131-139: Updated folder layout explanation

The explanation of the folder layout has been expanded to clarify the relationship between $DAEMON_HOME/cosmovisor, the application binaries, and the data directory. This provides better guidance for users setting up Cosmovisor.

However, consider adding a brief explanation of why a user might want to set $DAEMON_HOME to a different location than the data directory. This could help users understand the benefits of this flexibility.

🧰 Tools
🪛 Markdownlint

138-138: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


138-138: Add blank lines around code block

To improve readability and comply with Markdown best practices, add blank lines before and after the fenced code block.

Apply this change:

 Otherwise, if you decide to set `$DAEMON_HOME` to an arbitrary location, such as `$HOME/.simapp_manager`, and use the `--home` flag of the application as `$HOME/.simapp`, you must explicitly set the `DAEMON_DATA_DIR` to the application's data directory path, `$HOME/.simapp/data`. Your directory structure will look like this:
+
 ```text
 $HOME
  ├── .simapp
  │   ├── config
  │   └── data
  └── .simapp_manager
      └── cosmovisor

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

138-138: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used: .coderabbit.yml**
**Review profile: CHILL**

<details>
<summary>📥 Commits</summary>

Files that changed from the base of the PR and between de6f983b6b42e478efd33eb6012b4ca2a28fbe8f and 1f88eca525843cdcf707ca0b4bf0e309606fb117.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* tools/cosmovisor/README.md (2 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>📓 Path-based instructions (1)</summary><blockquote>

<details>
<summary>tools/cosmovisor/README.md (1)</summary>

Pattern `**/*.md`: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"


</details>

</blockquote></details>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

<details>
<summary>tools/cosmovisor/README.md</summary><blockquote>

138-138: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</blockquote></details>

</details>

<details>
<summary>🔇 Additional comments (2)</summary><blockquote>

<details>
<summary>tools/cosmovisor/README.md (2)</summary><blockquote>

`95-95`: **New configuration option added**

The new `DAEMON_DATA_DIR` option has been added, allowing users to set an absolute path to the `data` directory. This enhances flexibility in configuring Cosmovisor.

---

`140-148`: **Example directory structure added**

An example directory structure has been added to illustrate the separation of `$DAEMON_HOME` and the application's data directory. This visual representation enhances the explanation.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Cosmovisor Issues and PR related to Cosmovisor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Cosmovisor: Configurable Data directory path instead of hard-coded one
3 participants