-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(sidecar): configurable ValidatorIndex
#101
feat(sidecar): configurable ValidatorIndex
#101
Conversation
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.
Looks good! Left a few nits. Just one note: have you been able to test the CLI option?
I don't think that --validator-index 1, 3, 4
would work because of the spaces after each comma, but I'm not sure.
Also, it would be nice to support specifying an array with the [2,5] notation specified in the issue. One way to do this could be to check if the flag value starts with [
in the parse_validator_indexes function, but maybe there are other ways too
@merklefruit as discussed since the brackets are an issue, I have changed it to be compatible with quotation marks i.e. |
Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com>
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.
Great job with this one :)
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.
Looking good, small nit and updated some stuff as well
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.
Last nit, great job!
This PR adds CLI option to pass
validator_index
as comma-separated values i.e.--validator-index "1,2,3,4"
fixes #57