-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
beacon-chain: Reorganize flags in help text #14959
base: develop
Are you sure you want to change the base?
Conversation
…b, comment on db section
… to builder, comment on builder section
… sync, comment on sync section
…relevant to execution layer, comment on execution layer section
…ant to monitoring, comment on monitoring section
… to slasher, comment on slasher section
Fixed. |
344a574
to
6a77cb8
Compare
cmd/beacon-chain/usage.go
Outdated
cmd.E2EConfigFlag, | ||
cmd.GrpcMaxCallRecvMsgSizeFlag, | ||
cmd.MaxGoroutines, |
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.
cmd.MaxGoroutines
should go to debug
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.
yep, makes sense
Name: "cmd", | ||
Flags: []cli.Flag{ | ||
cmd.MinimalConfigFlag, | ||
cmd.AcceptTosFlag, | ||
cmd.ConfigFileFlag, |
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.
cmd.ConfigFileFlag
is here but there are other config flags in beacon-chain
:
cmd.ChainConfigFileFlag
(what's the difference between this one andcmd.ConfigFileFlag
?)cmd.E2EConfigFlag
flags.ChainID
flags.NetworkID
(what's the difference between this one andflags.ChainID
?)cmd.MinimalConfigFlag
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.
This doesn't feel right, but I am not entirely sure where they belong. The comment says Flags relevant to running the process
- which ones of these are relevant?
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.
The config file is used for setting all of the flags.
The other things you mentioned are for configuring the beacon chain so it feels more appropriate to keep them in the beacon chain section.
What type of PR is this?
Other
What does this PR do? Why is it needed?
The beacon-chain flags are all over the place in the current release.
The gist of this reorganization follows this loosely defined design:
The document above does not explain any ordering of the sections. I just ordered them in a way that made sense to me and feedback is welcome.
Important
Almost all of the changes are in the usage.go file for the beacon chain. However, there is one important change to review where the
--slasher
flag was removed from the feature flag set as it's a permanent feature and should be in the "slasher" section.v5.3.0 `--help` text
This PR `--help` text
Which issues(s) does this PR fix?
Other notes for review
Acknowledgements