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

cmd: simplify create cluster logic #2380

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Jun 30, 2023

This PR refactors create cluster logic to make it slightly simpler.

New behaviors:

  • if --split-existing-keys is enabled, --num-validators cannot be set
  • --num-validators must be present unless --split-existing-keys is enabled, otherwise create cluster exits with error
  • split key generation and disk read
  • don't fill in with random keys when splitting: during --split-existing-keys before this PR, if the user specified a --num-validators greater than the amount of keys contained in --split-keys-dir Charon would fill in the blanks with random keys
  • if definition file validator amount and --num-validators don't match, the creation process stops

category: refactor
ticket: #2341

Artificially set it to 1 if splitkeys is enabled, so that the code flow can continue.
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage: 67.56% and project coverage change: +0.04 🎉

Comparison is base (305a2e5) 53.63% compared to head (ffe2253) 53.67%.

❗ Current head ffe2253 differs from pull request most recent head 2c689c2. Consider uploading reports for the commit 2c689c2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2380      +/-   ##
==========================================
+ Coverage   53.63%   53.67%   +0.04%     
==========================================
  Files         192      192              
  Lines       25931    25958      +27     
==========================================
+ Hits        13907    13933      +26     
- Misses      10299    10306       +7     
+ Partials     1725     1719       -6     
Impacted Files Coverage Δ
cmd/createcluster.go 59.78% <67.56%> (+1.29%) ⬆️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

cmd/createcluster.go Outdated Show resolved Hide resolved
@OisinKyne
Copy link
Contributor

OisinKyne commented Jul 3, 2023

Artificially set it to 1 if --split-existing-keys is enabled and user didn't provide --num-validators, so that the code flow can continue.

What happens if I enable --split-existing-keys and point --split-existing-keys-dir at a folder with a number of keys in it? Will it produce only 1, or will it know to produce as many validators as there are keystores?

@gsora
Copy link
Collaborator Author

gsora commented Jul 3, 2023

I’m reworking the whole PR, I’ll ask for review later. The approach I used in this PR didn’t seem sound.

@gsora gsora changed the title cmd: default to 0 validators for create cluster cmd: simplify create cluster logic Jul 3, 2023
@gsora gsora requested a review from corverroos July 3, 2023 10:23
@gsora
Copy link
Collaborator Author

gsora commented Jul 3, 2023

@OisinKyne please review the behavior changes now

cmd/createcluster.go Outdated Show resolved Hide resolved
@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jul 3, 2023
@obol-bulldozer obol-bulldozer bot merged commit fef2ac7 into main Jul 3, 2023
@obol-bulldozer obol-bulldozer bot deleted the gsora/no_createcluster_valamt_default branch July 3, 2023 15:16
@gsora gsora linked an issue Jul 12, 2023 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove default values for critical flags
3 participants