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(cosmovisor): load cosmovisor configuration from toml file #19995

Merged
merged 22 commits into from
May 30, 2024

Conversation

akaladarshi
Copy link
Contributor

@akaladarshi akaladarshi commented Apr 10, 2024

Description

Closes: #19764

  • tools/cosmosvisor:
    • Added config flag and export-config
    • cinit command will export config file to a config.toml
    • run command will use config.toml to load config struct

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

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 configuration file support for better customization and management.
    • Enhanced the init command to automatically write configuration to a default path.
    • Added a new flag --cosmovisor-config to specify the config file path across various commands.
  • Enhancements

    • Updated commands to retrieve configurations from files, improving flexibility.
    • Added comprehensive error handling and validation for configurations.
    • Implemented new methods for exporting and importing configuration settings.
  • Refactor

    • Refactored command initialization and description enhancements in initCmd.
    • Updated runCmd to include detailed descriptions and handle new configuration parsing logic.
  • Bug Fixes

    • Adjusted handling of backup paths and configuration validations to prevent potential issues.

@github-actions github-actions bot added the C:Cosmovisor Issues and PR related to Cosmovisor label Apr 10, 2024
tools/cosmovisor/args.go Fixed Show fixed Hide fixed
@julienrbrt julienrbrt self-assigned this Apr 10, 2024
Copy link
Contributor

coderabbitai bot commented Apr 11, 2024

Walkthrough

The recent updates to the Cosmovisor tool focus on enhancing configuration management. Key changes include the integration of a configuration file system, improved error handling, and the ability to export and import configurations. Commands have been updated to support new configuration pathways, and a new flag has been introduced across various commands to specify the config file path, streamlining the tool's usability and flexibility.

Changes

File Path Change Summary
.../cosmovisor/args.go Enhanced Config struct, added configuration file handling, improved error handling and validation.
.../cosmovisor/CHANGELOG.md Documented the introduction of config file usage and related command updates.
.../cosmovisor/cmd/cosmovisor/*.go Updated command functionalities to support configuration from files, added new NewInitCmd() function, and introduced a persistent flag for configuration file path.
.../cosmovisor/cmd/cosmovisor/run.go Enhanced to parse the config file path from command args and updated function signatures to support this.
.../cosmovisor/cmd/cosmovisor/version.go Updated function calls to include new parameter handling.
.../cosmovisor/flags.go Added a new constant for the Cosmovisor configuration file flag.

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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

@akaladarshi akaladarshi marked this pull request as ready for review April 11, 2024 15:54
@akaladarshi akaladarshi requested a review from a team as a code owner April 11, 2024 15:54
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: 1

Comment on lines 25 to 28
func mustNoError(err error) {
if err != nil {
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider enhancing the mustNoError function by adding logging before panicking to provide more context about the error, which can be helpful for debugging.

func mustNoError(err error) {
	if err != nil {
+		log.Error("An error occurred:", err)
		panic(err)
	}
}

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func mustNoError(err error) {
if err != nil {
panic(err)
}
func mustNoError(err error) {
if err != nil {
log.Error("An error occurred:", err)
panic(err)
}
}

@akaladarshi
Copy link
Contributor Author

Hey @julienrbrt.

Should I mark these changes under version v1.6.0 in the ChangeLog?

@julienrbrt
Copy link
Member

Hi, you can just leave it under unreleased. A new version of cosmovisor is due soon but there is other issues to fix before tagging a new version.

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: 5

Comment on lines 44 to 46
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adjust the indentation of the unordered list items to align with the markdown standards (4 spaces instead of 2).

-  * Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
+    * Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
-  * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
+    * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
-  * Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
+    * Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.

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: 5

@@ -36,6 +36,17 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

## Features
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure there is a blank line before the "Features" heading to maintain consistency and readability.

+ 
## Features

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
## Features
## Features

@@ -36,6 +36,17 @@

## [Unreleased]

## Features

* [#19764](https://github.com/cosmos/cosmos-sdk/issues/19764) Use config file for cosmovisor configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure there is a blank line after the "Features" list item to separate it from the next section for better readability.

* [#19764](https://github.com/cosmos/cosmos-sdk/issues/19764) Use config file for cosmovisor configuration.
+ 

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#19764](https://github.com/cosmos/cosmos-sdk/issues/19764) Use config file for cosmovisor configuration.
* [#19764](https://github.com/cosmos/cosmos-sdk/issues/19764) Use config file for cosmovisor configuration.


* [#19764](https://github.com/cosmos/cosmos-sdk/issues/19764) Use config file for cosmovisor configuration.

## Improvements
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure there is a blank line before the "Improvements" heading to maintain consistency and readability.

+ 
## Improvements

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
## Improvements
## Improvements


## Improvements

* [#19995](https://github.com/cosmos/cosmos-sdk/pull/19995):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure there is a blank line after the "Improvements" list item to separate it from the next section for better readability.

* [#19995](https://github.com/cosmos/cosmos-sdk/pull/19995):
+ 

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#19995](https://github.com/cosmos/cosmos-sdk/pull/19995):
* [#19995](https://github.com/cosmos/cosmos-sdk/pull/19995):

Comment on lines 46 to 48
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adjust the indentation of the unordered list items to align with markdown standards (4 spaces instead of 2).

-    * Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
+        * Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
-    * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
+        * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
-    * Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
+        * Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.
* Add `--export-config` flag to provide `config.toml` path to initialise configuration in `cosmovisor init`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`
* Add `--config` flag to provide `config.toml` path to the configuration file in `cosmovisor add-upgrade`.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Great work!! I have left a few comments

TimeFormatLogs string
CustomPreupgrade string
DisableRecase bool
Home string `toml:"DAEMON_HOME" mapstructure:"DAEMON_HOME"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we have the toml keys lower-case and kebab-case? This looks odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// If ENV variables are set, they will override the values in the file.
func GetConfigFromFile(filePath string) (*Config, error) {
if filePath == "" {
return nil, ErrEmptyConfigENV
Copy link
Member

@julienrbrt julienrbrt Apr 12, 2024

Choose a reason for hiding this comment

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

I don't think we should return an error, that will be a behavior change for existing users. Maybe we should use a temporary empty file? Quite hacky, but at least it will preserve the current behavior for users that won't use the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

Updated. Now if the filePath is not provided instead of returning an error it will try to load config from env variables


return addUpgrade
}

// AddUpgrade adds upgrade info to manifest
func AddUpgrade(cmd *cobra.Command, args []string) error {
cfg, err := cosmovisor.GetConfigFromEnv()
configPath, err := cmd.Flags().GetString(cosmovisor.FlagConfig)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we read the config in the root command and pass it down to sub commands?
this way we can have a persistent flag on the root command and avoid adding a flag to each command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used two flags:

  • config for taking config file as input used by (add-upgrade)
  • export-config for exporting configuration to toml file used by (init).

Using config flag in root command will create confusion for user as they will see both flags export-config and config for init command.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don't think we should allow to export during init. Imho init should always create the file (maybe ask for a confirmation if the file already exists).

Additionally for people upgrading having an export command that converts flags to a config flag would be handy.

Additionally this way you don't have clashing flag names as you just pointed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.
So basically init command should only write to default path(leaving file already exist case aside).

additionally export command can read the config from the file(override if env is set) and print it.

Copy link
Member

Choose a reason for hiding this comment

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

yes, sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

},
}

initCmd.Flags().String(cosmovisor.FlagExportConfig, "", "path to export the configuration file to (default is <-home_path->/cosmovisor/config.toml")
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where the default is set.
IMHO it shouldn't be current dir but within the node directory (DAEMON_HOME)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is being set here.

I agree with you that's why I didn't set default path directly in flag because it depends on DEAMON_HOME so it has to be loaded at run time.

The default path is: DEAMON_HOME/COSMOVISOR/config.toml, you can check here.

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 it, thanks!

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: 3

Comment on lines +97 to +103
filePath, err := cfg.Export()
if err != nil {
return fmt.Errorf("failed to export configuration: %w", err)
}

logger.Info(fmt.Sprintf("config file present at: %s", filePath))

Copy link
Contributor

Choose a reason for hiding this comment

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

Validate the configuration export path before proceeding.

Before exporting the configuration, validate that the path is writable and not already occupied by crucial files. This prevents potential data loss by ensuring that the export operation does not overwrite important files unintentionally.

Comment on lines 46 to 49
* `init command` writes the configuration to the config file only at the default path `DAEMON_HOME/cosmovisor/config.toml`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`.
* Add `--config` flag to provide `config.toml` path to the configuration file in root command used by `add-upgrade` and `config` subcommands.
* `config command` now displays the configuration from the config file if it is provided. If the config file is not provided, it will display the configuration from the environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adjust the indentation of list items to align with Markdown standards.

-    * `init command` writes the configuration to the config file only at the default path `DAEMON_HOME/cosmovisor/config.toml`.
+  * `init command` writes the configuration to the config file only at the default path `DAEMON_HOME/cosmovisor/config.toml`.
-    * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`.
+  * Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`.
-    * Add `--config` flag to provide `config.toml` path to the configuration file in root command used by `add-upgrade` and `config` subcommands.
+  * Add `--config` flag to provide `config.toml` path to the configuration file in root command used by `add-upgrade` and `config` subcommands.
-    * `config command` now displays the configuration from the config file if it is provided. If the config file is not provided, it will display the configuration from the environment variables.
+  * `config command` now displays the configuration from the config file if it is provided. If the config file is not provided, it will display the configuration from the environment variables.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* `init command` writes the configuration to the config file only at the default path `DAEMON_HOME/cosmovisor/config.toml`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`.
* Add `--config` flag to provide `config.toml` path to the configuration file in root command used by `add-upgrade` and `config` subcommands.
* `config command` now displays the configuration from the config file if it is provided. If the config file is not provided, it will display the configuration from the environment variables.
* `init command` writes the configuration to the config file only at the default path `DAEMON_HOME/cosmovisor/config.toml`.
* Set `CONFIG` environment variable to provide `config.toml` path to load the configuration in `cosmovisor run`.
* Add `--config` flag to provide `config.toml` path to the configuration file in root command used by `add-upgrade` and `config` subcommands.
* `config command` now displays the configuration from the config file if it is provided. If the config file is not provided, it will display the configuration from the environment variables.

tools/cosmovisor/args.go Outdated Show resolved Hide resolved
@akaladarshi akaladarshi requested a review from julienrbrt April 15, 2024 04:07
@julienrbrt julienrbrt mentioned this pull request Apr 17, 2024
7 tasks
@julienrbrt
Copy link
Member

Thanks for this, I'll review today!

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Amazing work, left some comments

runCmd,
configCmd,
NewVersionCmd(),
NewAddUpgradeCmd(),
)

rootCmd.PersistentFlags().String(cosmovisor.FlagConfig, "", "path to cosmovisor config file")
Copy link
Member

Choose a reason for hiding this comment

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

Nit, StringP and use c as shortcut

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -18,7 +27,7 @@ var runCmd = &cobra.Command{

// run runs the configured program with the given args and monitors it for upgrades.
func run(args []string, options ...RunOption) error {
cfg, err := cosmovisor.GetConfigFromEnv()
cfg, err := cosmovisor.GetConfigFromFile(os.Getenv(cfgENV))
Copy link
Member

Choose a reason for hiding this comment

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

I don't get this, why cannot we use the --config flag from cosmovisor.
I don't like that you need another env variable just for the config name.

Copy link
Contributor Author

@akaladarshi akaladarshi Apr 20, 2024

Choose a reason for hiding this comment

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

As you can see here run command has disabled flag parsing.
So I didn't want to change the existing behaviour of the run command.

edit:
As of now cosmovisor run -h will not show any flags which might create confusion.

Let me know your thoughts on this I will update it.

Copy link
Member

Choose a reason for hiding this comment

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

That is true. Then I think we should maybe rename the --config flag so we are sure it won't clash with an app flag, maybe --cosmovisor-config. Then I'd personally say we should parse that one for consistency here too.
It will a tiny be less nice to implement but the UX will be better.
Are you willing to do that? Otherwise happy to do that myself in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.
So you mean we should replace the config flag with cosmovisor-config in root commands and enable flag parsing in run command.

Copy link
Member

@julienrbrt julienrbrt Apr 20, 2024

Choose a reason for hiding this comment

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

Yes, but you shouldn't enable flag parsing as you pass the app command there. You'll need to manually parse --cosmovisor-config there and remove it from the arguments you pass to the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tools/cosmovisor/cmd/cosmovisor/run.go Show resolved Hide resolved
@akaladarshi akaladarshi requested a review from julienrbrt April 23, 2024 09:31
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: 3

Out of diff range and nitpick comments (4)
tools/cosmovisor/args.go (4)

160-202: Review the error handling in GetConfigFromFile. Consider aggregating errors rather than returning immediately to provide more comprehensive feedback to the user.


160-202: The function GetConfigFromFile should handle the case where the configuration file is not found more gracefully. Perhaps it could fall back to environment variables if the file is not mandatory.


465-465: The getTimeFormatOption function currently returns an error if an unrecognized format is provided. It might be more user-friendly to log a warning and fall back to a default format.


577-609: The Export function should handle potential errors from file.Close() in the deferred function by logging them or handling them in another appropriate manner.

Comment on lines +55 to +70
Home string `toml:"daemon_home" mapstructure:"daemon_home"`
Name string `toml:"daemon_name" mapstructure:"daemon_name"`
AllowDownloadBinaries bool `toml:"daemon_allow_download_binaries" mapstructure:"daemon_allow_download_binaries" default:"false"`
DownloadMustHaveChecksum bool `toml:"daemon_download_must_have_checksum" mapstructure:"daemon_download_must_have_checksum" default:"false"`
RestartAfterUpgrade bool `toml:"daemon_restart_after_upgrade" mapstructure:"daemon_restart_after_upgrade" default:"true"`
RestartDelay time.Duration `toml:"daemon_restart_delay" mapstructure:"daemon_restart_delay"`
ShutdownGrace time.Duration `toml:"daemon_shutdown_grace" mapstructure:"daemon_shutdown_grace"`
PollInterval time.Duration `toml:"daemon_poll_interval" mapstructure:"daemon_poll_interval" default:"300ms"`
UnsafeSkipBackup bool `toml:"unsafe_skip_backup" mapstructure:"unsafe_skip_backup" default:"false"`
DataBackupPath string `toml:"daemon_data_backup_dir" mapstructure:"daemon_data_backup_dir"`
PreUpgradeMaxRetries int `toml:"daemon_preupgrade_max_retries" mapstructure:"daemon_preupgrade_max_retries" default:"0"`
DisableLogs bool `toml:"cosmovisor_disable_logs" mapstructure:"cosmovisor_disable_logs" default:"false"`
ColorLogs bool `toml:"cosmovisor_color_logs" mapstructure:"cosmovisor_color_logs" default:"true"`
TimeFormatLogs string `toml:"cosmovisor_timeformat_logs" mapstructure:"cosmovisor_timeformat_logs" default:"kitchen"`
CustomPreUpgrade string `toml:"cosmovisor_custom_preupgrade" mapstructure:"cosmovisor_custom_preupgrade" default:""`
DisableRecase bool `toml:"cosmovisor_disable_recase" mapstructure:"cosmovisor_disable_recase" default:"false"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the TOML tags are in kebab-case as per previous review comments.

return getTimeFormatOption(val)
}

func getTimeFormatOption(val string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The getTimeFormatOption function should handle an empty string case more gracefully by returning a default or previously set value instead of an empty string, which might not be a valid time format.

Comment on lines +577 to +609
// Export exports the configuration to a file at the given path.
func (cfg Config) Export() (string, error) {
// always use the default path
path := filepath.Clean(cfg.DefaultCfgPath())

// check if config file already exists ask user if they want to overwrite it
if _, err := os.Stat(path); err == nil {
// ask user if they want to overwrite the file
if !askForConfirmation(fmt.Sprintf("file %s already exists, do you want to overwrite it?", path)) {
cfg.Logger(os.Stdout).Info("file already exists, not overriding")
return path, nil
}
}

// create the file
file, err := os.Create(filepath.Clean(path))
if err != nil {
return "", fmt.Errorf("failed to create configuration file: %w", err)
}

// convert the time value to its format option
cfg.TimeFormatLogs = ValueToTimeFormatOption(cfg.TimeFormatLogs)

defer file.Close()

// write the configuration to the file
err = toml.NewEncoder(file).Encode(cfg)
if err != nil {
return "", fmt.Errorf("failed to encode configuration: %w", err)
}

return path, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Export function, the confirmation prompt logic could be improved. Consider moving the prompt into a separate function to clean up the Export function and make it more readable.

- if !askForConfirmation(fmt.Sprintf("file %s already exists, do you want to overwrite it?", path)) {
-     cfg.Logger(os.Stdout).Info("file already exists, not overriding")
-     return path, nil
- }
+ if fileExists(path) {
+     if !confirmOverwrite(path) {
+         cfg.Logger(os.Stdout).Info("file already exists, not overriding")
+         return path, nil
+     }
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// Export exports the configuration to a file at the given path.
func (cfg Config) Export() (string, error) {
// always use the default path
path := filepath.Clean(cfg.DefaultCfgPath())
// check if config file already exists ask user if they want to overwrite it
if _, err := os.Stat(path); err == nil {
// ask user if they want to overwrite the file
if !askForConfirmation(fmt.Sprintf("file %s already exists, do you want to overwrite it?", path)) {
cfg.Logger(os.Stdout).Info("file already exists, not overriding")
return path, nil
}
}
// create the file
file, err := os.Create(filepath.Clean(path))
if err != nil {
return "", fmt.Errorf("failed to create configuration file: %w", err)
}
// convert the time value to its format option
cfg.TimeFormatLogs = ValueToTimeFormatOption(cfg.TimeFormatLogs)
defer file.Close()
// write the configuration to the file
err = toml.NewEncoder(file).Encode(cfg)
if err != nil {
return "", fmt.Errorf("failed to encode configuration: %w", err)
}
return path, nil
}
// Export exports the configuration to a file at the given path.
func (cfg Config) Export() (string, error) {
// always use the default path
path := filepath.Clean(cfg.DefaultCfgPath())
// check if config file already exists ask user if they want to overwrite it
if _, err := os.Stat(path); err == nil {
// ask user if they want to overwrite the file
if fileExists(path) {
if !confirmOverwrite(path) {
cfg.Logger(os.Stdout).Info("file already exists, not overriding")
return path, nil
}
}
}
// create the file
file, err := os.Create(filepath.Clean(path))
if err != nil {
return "", fmt.Errorf("failed to create configuration file: %w", err)
}
// convert the time value to its format option
cfg.TimeFormatLogs = ValueToTimeFormatOption(cfg.TimeFormatLogs)
defer file.Close()
// write the configuration to the file
err = toml.NewEncoder(file).Encode(cfg)
if err != nil {
return "", fmt.Errorf("failed to encode configuration: %w", err)
}
return path, nil
}


## Improvements

* [#19995](https://github.com/cosmos/cosmos-sdk/pull/19995):
Copy link
Member

Choose a reason for hiding this comment

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

Nice description here, could you as well update the README with almost a copy paste of that :D

Copy link
Member

Choose a reason for hiding this comment

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

I'll do this in a follow-up then, as I am touching cosmovisor. Merging now.

@julienrbrt julienrbrt added this pull request to the merge queue May 30, 2024
Merged via the queue into cosmos:main with commit 465410c May 30, 2024
61 of 63 checks passed
alpe added a commit that referenced this pull request May 31, 2024
* main:
  feat(server/v2): introduce cometbft v2 (#20483)
  refactor(x/upgrade): migrate to appmodule.VersionMap (#20485)
  docs: code guidelines changes (#20482)
  feat(cosmovisor): load cosmovisor configuration from toml file (#19995)
  perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034)
  fix: Bump CometBFT versions (#20481)
alpe added a commit that referenced this pull request May 31, 2024
* main: (120 commits)
  chore: update protoc-gen-swagger to protoc-gen-openapiv2 (#20448)
  ci: Add GitHub Action for go mod tidy and mocks (#20501)
  chore: Address linter issues (#20486)
  fix: wrap errors in auto CLI service registration (#20493)
  chore: fix comment (#20498)
  chore: fix the note box syntax error (#20499)
  feat(server/v2): introduce cometbft v2 (#20483)
  refactor(x/upgrade): migrate to appmodule.VersionMap (#20485)
  docs: code guidelines changes (#20482)
  feat(cosmovisor): load cosmovisor configuration from toml file (#19995)
  perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034)
  fix: Bump CometBFT versions (#20481)
  refactor(core): remove redundant ExecMode (#20322)
  feat(store/v2): pruning manager (#20430)
  chore: force reload sonar cloud  (#20480)
  refactor(x/accounts): reuse calculated sum in `func safeAdd` (#20458)
  refactor: remove `defer` in loop (#20223)
  ci: remove livness test (#20474)
  build(deps): Bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#20477)
  chore: migrate a few diagrams to mermaid (#20473)
  ...
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]: Take configuration from config file instead of env vars on Cosmovisor
4 participants