Skip to content

Conversation

@MikeJerred
Copy link

@MikeJerred MikeJerred commented Oct 30, 2025

Issue Addressed

#3768

Proposed Changes

Made the --validator-dir flag global so that it can be specified in any order

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@cla-assistant
Copy link

cla-assistant bot commented Oct 30, 2025

CLA assistant check
All committers have signed the CLA.

@chong-he
Copy link
Member

There is a CI check that fails. May not be related to your change, maybe need to update to latest unstable, can you fix it? Thanks

@chong-he chong-he self-requested a review October 31, 2025 14:19
Copy link
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this PR is a 1-line change in the main code, I think the PR has merits in the test file change, as commented below.

Putting a note here that this change will be replaced by the clap derive migration PR #6493, where we can add a global = true here cc @eserilev

I have tested in both cases and it works


let validator_import_key_cmd = || {
validator_cmd()
.arg(IMPORT_CMD)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is good, as it moves the subcommand import (and other subcommands) to before the --validator-dir flag.

The test will only pass with the change in mod.rs to add .global(true) to allow subcommands such as import be placed before the --valdiator-dir

"The path to search for validator directories. \
Defaults to ~/.lighthouse/{network}/validators",
)
.global(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if there is a better way than adding .global(true) here, because the --validator-dir flag is defined before the subcommands import, create etc. So I think adding a global here is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants