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

Investigate and update CLI #3163

Closed
timwu20 opened this issue Mar 17, 2023 · 6 comments
Closed

Investigate and update CLI #3163

timwu20 opened this issue Mar 17, 2023 · 6 comments
Assignees

Comments

@timwu20
Copy link
Contributor

timwu20 commented Mar 17, 2023

Issue summary

Other information and links

@timwu20 timwu20 changed the title Investigate and Update CLI Investigate and update CLI Mar 17, 2023
@kanishkatn
Copy link
Contributor

kanishkatn commented Mar 20, 2023

Should we continue using urfave/cli

Advantages of using cobra over urfave/cli

  1. cobra is feature rich
  2. cobra uses a hierarchical configuration system that allows us to define commands, sub-commands, and flags using a tree-like structure.
  3. cobra has a larger and more active community of users and contributors, as well as more extensive documentation and examples.
  4. Easy to integrate with viper
  5. urfave/cli does not enforce a strict command structure, which can lead to inconsistencies across different commands and flags.
  6. urfave/cli has limited support for command-line completions
  7. urfave/cli does not have built-in support for parsing complex command-line arguments, such as lists or dictionaries.

Drawbacks of cobra

  1. It is more complex than urfave/cli
  2. cobra has a larger codebase and more dependencies than some other CLI packages, which can increase the size and complexity of the project.
  3. While cobra is highly customizable, it can be more difficult to customize than some other CLI packages. Customizing the behavior of commands and flags often requires writing custom code, which can be time-consuming and error-prone.
  4. cobra has a large number of dependencies, which can make it more difficult to manage and maintain the project.
  5. cobra’s powerful API can make it easy to write complex and feature-rich CLIs, but this can also lead to code bloat. If we are not careful, the codebase can quickly become bloated and difficult to maintain.

Projects using cobra

  1. Tendermint
  2. Web3Auth ( shameless plug )
  3. Twitch CLI
  4. Github CLI
  5. Kubernetes
  6. Cosmos SDK
  7. Docker

I personally prefer cobra and feel it will be very useful in the long run, since it can support all the features we’d need.

I also believe we shouldn’t fix something that isn’t broken.

Are we facing any issues with urfave/cli?

@kanishkatn
Copy link
Contributor

Investigate whether or not we still require TOML parsing. For context, none of the other nodes (substrate, smoldot) support this.

Currently, the project has very few settings that seems unlikely to change. The TOML config file is mostly used to define path to a chain’s genesis file. It is definitely underused.

However, it does provide the following benefits:

  • Using a config file is user friendly
  • Since we are supporting multiple chains, it will be easy to configure settings specific to a chain
  • Using a configuration file can provide a history of changes that can be tracked and managed using version control.
  • A configuration file can provide a common interface for developers to share and manage settings, reducing the risk of misconfigurations.
  • A configuration file can act as a documentation for the settings

Although, our TOML config is underused at the moment, I feel it could be beneficial in the long run.

@kanishkatn
Copy link
Contributor

I discussed this with Ed and went through the current implementation of the cli and the config management. We both felt it could use a refactor to make it more readable and maintainable.

Here are the changes that’ll need to be done as part of the refactor:

  • Replace urfave/cli with cobra
  • Use viper for config management
  • Refactor Config to work seamlessly with cobra and viper
  • Look into introducing ENVs for few basic settings like GSSMRHOME
  • Use the same flags and nomenclature as that of Substrate

I have started working on it!

@qdm12
Copy link
Contributor

qdm12 commented Mar 31, 2023

Gotta add my 2 cents to this:

  1. We should (must?) have at least 2 sources of settings, ideally for all settings. It then forces us to have modular code to support multiple sources.
  2. We should/must pay attention to precedence of settings sources, i.e. flags > toml > env variables and that for all settings (we don't want to deal with precedence field by field which is highly error prone)
  3. Flags is definitely a must have, the second one can stay as toml (config files are useful / can be loaded as secrets more easily than flags).
  4. If we want to add environment variables, I would suggest to support all the settings with them and not just a few of them. That can be done after migrating to cobra and doesn't feel like such a priority. It can become more pressing once we have a lot more users running gossamer in a container/k8s since it's friendlier with env variables (i.e. as well as UIs like Portainer, Unraid etc.)

@kanishkatn
Copy link
Contributor

kanishkatn commented May 23, 2023

Update:

We have replaced urfave/cli with cobra and viper here #3173

Pending tasks:

@kanishkatn kanishkatn mentioned this issue Nov 18, 2023
5 tasks
@kanishkatn
Copy link
Contributor

Closing this and tracking the further improvements in #3590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants