feat: rename prover CLI start command as proxy and make it default #6428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6428 +/- ##
============================================
- Coverage 61.72% 61.70% -0.03%
============================================
Files 553 553
Lines 57862 57858 -4
Branches 1829 1829
============================================
- Hits 35717 35702 -15
- Misses 22108 22119 +11
Partials 37 37 |
nflaig
reviewed
Feb 14, 2024
There was a problem hiding this comment.
Doesn't this restrict the extensibility of the CLI as it will match any subcommand?
I'd agree that start is not a good name for the sub command, maybe just rename it to proxy? We also need to be mindful when renaming commands as it breaks current usage but I don't know of anybody even uses the prover as a proxy server.
Performance Report✔️ no performance regression detected Full benchmark results
|
|
I am under the impression that there are no other commands planned in the foreseeable future. Maybe @nazarhussain can comment on that. |
My plans were to provide some utility commands like But don't have any planned schedule for such features. |
Great ideas! Ok, I will just rename to |
nazarhussain
requested changes
Feb 14, 2024
jeluard
force-pushed
the
jeluard/prover-start-command
branch
from
06c7f6a
to
be84c20
Compare
nflaig
reviewed
Feb 15, 2024
packages/prover/src/cli/cli.ts
Outdated
| @@ -48,6 +48,9 @@ export function getLodestarProverCli(): yargs.Argv { | |||
| registerCommandToYargs(prover, cmd); | |||
| } | |||
|
|
|||
| // Register the proxy command as the default one | |||
| registerCommandToYargs(prover, {...proverProxyStartCommand, command: '*'}); | |||
There was a problem hiding this comment.
What happens if you run lodestar-prover --help, I would assume it no longer prints out global flags and a list of available commands.
There was a problem hiding this comment.
It prints the global help screen, as before. lodestar-prover proxy --help prints the help for the proxy command as expected.
nazarhussain
approved these changes
Feb 16, 2024
There was a problem hiding this comment.
LGTM.
That will be a breaking change, we need to discuss and finalize the release strategy for prover in such cases. Opened a discussion point for next meeting. #6434 (comment)
|
The PR description and title needs to be updated to properly reflect the changes. We might also wanna label this PR as breaking change and add a BREAKING CHANGE note as well with the changes required |
jeluard
changed the title
nflaig
added
the
meta-breaking-change
nflaig
approved these changes
Feb 16, 2024
|
🎉 This PR is included in v1.16.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
proverCLI has a single command,start, rename it asproxyand make it the default for better UXDescription
It's now possible to start the prover using
npx @chainsafe/prover proxy ...ornpx @chainsafe/prover ...(previously wasnpx @chainsafe/prover start ...).This is a breaking change as the old
startcommand won't work anymore.