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

refactor(cli): replace urfave/cli with cobra #3173

Merged
merged 138 commits into from
May 17, 2023
Merged

Conversation

kanishkatn
Copy link
Contributor

@kanishkatn kanishkatn commented Mar 26, 2023

Changes

  • Replace urfave/cli with cobra
  • Integrate viper for config management
  • Use the substrate flags
  • Add docs to the toml config file
  • Refactor the base-path
    • config.toml is stored in base-path/config/
    • DBs are stored in base-path/data/ directory
    • base-path can be set in ENV as GSSMR_HOME
  • Move the config from ./dot/tomlconfig to ./config
  • Add the config basic validations in the cli
  • Rename common.Roles to common.NetworkRole
  • Update all the tests to cater to the new Config structure

Tests

Since the changes affect the entire node, we'll have to make sure all the tests in the project are passing.

I have run the unit tests, integration tests and have made sure all the CI checks are passing.

To run a node:

Method 1

  • Generate the default config.toml by running
gossamer init --chain westend --base-path ~/.gossamer/westend

This creates the working directory in the base-path with the following

  • A default config.toml at base-path/config for the specified chain
  • Sets up the databases at base-path/data directory
    Then
  • Modify the config.toml
  • Run the node with
gossamer --base-path ~/.gossamer/westend

Method 2

  • Run the node directly via
gossamer --chain westend --port 7000 --key alice
  • Use the gossamer help command to see all the supported flags

Issues

#3163

TODO:

  • Improve the toml docs ( help needed )
  • Update docs
  • Check the default config values and improve basic validation ( help needed )
  • Add a flag for telemetry-url
  • Add a flag for role
  • Write additional unit tests for the cmd package
  • Figure out the issue with TestStableNetworkRPC Investigate TestStableNetworkRPC #3212

Future Improvements:

Primary Reviewer

@timwu20 @edwardmack

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

❗ No coverage uploaded for pull request base (development@779608e). Click here to learn what that means.
The diff coverage is 41.16%.

Additional details and impacted files
@@              Coverage Diff               @@
##             development    #3173   +/-   ##
==============================================
  Coverage               ?   51.83%           
==============================================
  Files                  ?      225           
  Lines                  ?    28549           
  Branches               ?        0           
==============================================
  Hits                   ?    14797           
  Misses                 ?    12366           
  Partials               ?     1386           

@qdm12
Copy link
Contributor

qdm12 commented Mar 30, 2023

Forgive me I'm still "out of the loop" for now, but what's the reason for changing cli framework? (I have no preference for either framework, so just being curious 😉)

@kanishkatn
Copy link
Contributor Author

kanishkatn commented Mar 30, 2023

Forgive me I'm still "out of the loop" for now, but what's the reason for changing cli framework? (I have no preference for either framework, so just being curious 😉)

@qdm12 Hey there, here's the issue for this #3163

@jimjbrettj jimjbrettj self-requested a review May 3, 2023 18:32
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

Had one comment on the issue you created and why its needed, but besides that all looks great! Really awesome job wth this

Copy link
Contributor

@timwu20 timwu20 left a comment

Choose a reason for hiding this comment

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

Pulled the branch and double checked the entrypoint on Dockerfile.staging.

I tried running ./bin/gossamer --chain=westend --pprof.enabled but I'm getting an error on no key specified. The account key should be optional, since gossamer will create one if not specified. This is used for non-authority syncing nodes.

@kanishkatn
Copy link
Contributor Author

Pulled the branch and double checked the entrypoint on Dockerfile.staging.

I tried running ./bin/gossamer --chain=westend --pprof.enabled but I'm getting an error on no key specified. The account key should be optional, since gossamer will create one if not specified. This is used for non-authority syncing nodes.

That isn't the current behaviour. It defaults to alice if the key is empty.

Let's default it to alice for now and generate the key in another PR?

@timwu20
Copy link
Contributor

timwu20 commented May 11, 2023

Pulled the branch and double checked the entrypoint on Dockerfile.staging.
I tried running ./bin/gossamer --chain=westend --pprof.enabled but I'm getting an error on no key specified. The account key should be optional, since gossamer will create one if not specified. This is used for non-authority syncing nodes.

That isn't the current behaviour. It defaults to alice if the key is empty.

Let's default it to alice for now and generate the key in another PR?

Oh is it? Yes let's default to alice for now and make the flag optional.

@kanishkatn
Copy link
Contributor Author

Oh is it? Yes let's default to alice for now and make the flag optional.

It's done!

@timwu20
Copy link
Contributor

timwu20 commented May 12, 2023

Looks like something is failing with BABE when trying to run the node on this branch.

timothywu@Timothys-MacBook-Pro gossamer % ./bin/gossamer --chain=westend --pprof.enabled                      
2023-05-11T21:40:17-04:00 INFO     🕸️ initialising node services with global configuration name Westend, id gssmr and base path /Users/timothywu/.gossamer/westend...    pkg=dot
2023-05-11T21:40:18-04:00 INFO     created state service with head 0xf473d4edf85d5acefbf08152a3549f1353290a888e9939aa5edd964c37044693, highest number 1024 and genesis hash 0xe143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e  pkg=state
2023-05-11T21:40:18-04:00 WARN     Bootstrap is enabled but no bootstrap nodes are defined      pkg=network
2023-05-11T21:40:21-04:00 INFO     creating runtime with interpreter wasmer...  pkg=dot
2023-05-11T21:40:22-04:00 INFO     instantiated runtime!!!      pkg=dot
2023-05-11T21:40:22-04:00 INFO     registered notifications sub-protocol /e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e/grandpa/1    pkg=network
2023-05-11T21:40:22-04:00 INFO     creating BABE service as authority...        pkg=dot
2023-05-11T21:40:22-04:00 INFO     keystore with keys [0xc00117f690]    pkg=dot
2023-05-11T21:40:22-04:00 INFO     starting node Westend...     pkg=cmd
2023-05-11T21:40:22-04:00 INFO     🕸️ starting node services...  pkg=dot
2023-05-11T21:40:22-04:00 INFO     Starting services: [*system.Service *network.Service *digest.Handler *core.Service *grandpa.Service *sync.Service *babe.Service *state.Service]      pkg=dot,services
2023-05-11T21:40:22-04:00 INFO     registered notifications sub-protocol /gossamer/gssmr/0/block-announces/1    pkg=network
2023-05-11T21:40:22-04:00 INFO     registered notifications sub-protocol /gossamer/gssmr/0/transactions/1       pkg=network
2023-05-11T21:40:22-04:00 INFO     Started listening on /ip4/127.0.0.1/tcp/7001/p2p/12D3KooWDKECFWrsafyKHb3wgYYV548f4QjcMoKbQBdQpk5NHwFp        pkg=network
2023-05-11T21:40:22-04:00 INFO     Started listening on /ip4/74.12.251.190/tcp/7001/p2p/12D3KooWDKECFWrsafyKHb3wgYYV548f4QjcMoKbQBdQpk5NHwFp    pkg=network
2023-05-11T21:40:22-04:00 INFO     started network service with supported protocols /ipfs/ping/1.0.0, /ipfs/id/1.0.0, /ipfs/id/push/1.0.0, /e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e/grandpa/1, /gossamer/gssmr/0/sync/2, /gossamer/gssmr/0/light/2, /gossamer/gssmr/0/block-announces/1, /gossamer/gssmr/0/transactions/1  pkg=network
2023-05-11T21:40:22-04:00 CRITICAL failed to run block production engine: cannot handle epoch: cannot initiate and get epoch handler: failed to initiate epoch: cannot get epoch data and start slot: cannot get latest epoch data: cannot get authority index: key not in BABE authority data      pkg=babe

@kanishkatn kanishkatn force-pushed the refactor/k/cli branch 4 times, most recently from eabd6a8 to 000b9b4 Compare May 12, 2023 13:07
@kanishkatn
Copy link
Contributor Author

kanishkatn commented May 12, 2023

Looks like something is failing with BABE when trying to run the node on this branch.

It was an issue with the DefaultConfig. Thank you!
I've fixed that and an issue with viper, and ran the node for all the supported networks.

@kanishkatn kanishkatn merged commit db0be3f into development May 17, 2023
@kanishkatn kanishkatn deleted the refactor/k/cli branch May 17, 2023 05:33
@kanishkatn kanishkatn mentioned this pull request Nov 18, 2023
5 tasks
Copy link

🎉 This PR is included in version 0.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants