-
Notifications
You must be signed in to change notification settings - Fork 129
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## development #3173 +/- ##
==============================================
Coverage ? 51.83%
==============================================
Files ? 225
Lines ? 28549
Branches ? 0
==============================================
Hits ? 14797
Misses ? 12366
Partials ? 1386 |
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 😉) |
There was a problem hiding this 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
There was a problem hiding this 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.
That isn't the current behaviour. It defaults to Let's default it to |
Oh is it? Yes let's default to alice for now and make the flag optional. |
It's done! |
Looks like something is failing with BABE when trying to run the node on this branch.
|
eabd6a8
to
000b9b4
Compare
000b9b4
to
d8e62fb
Compare
It was an issue with the |
🎉 This PR is included in version 0.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes
urfave/cli
withcobra
viper
for config managementsubstrate
flagstoml
config filebase-path
config.toml
is stored inbase-path/config/
base-path/data/
directorybase-path
can be set in ENV asGSSMR_HOME
./dot/tomlconfig
to./config
common.Roles
tocommon.NetworkRole
Config
structureTests
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
config.toml
by runningThis creates the working directory in the
base-path
with the followingconfig.toml
atbase-path/config
for the specified chainbase-path/data
directoryThen
config.toml
Method 2
gossamer help
command to see all the supported flagsIssues
#3163
TODO:
toml
docs ( help needed )telemetry-url
role
TestStableNetworkRPC
InvestigateTestStableNetworkRPC
#3212Future Improvements:
data
directory for the databases cli: config improvements for base-path #3211*config/Config
and*dot/Config
#1848sync
service Use separate peer config for sync service #3240Primary Reviewer
@timwu20 @edwardmack