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

Change * value to 'all' for improved bash compatibility ( Issue #5963 ) #6459

Closed
wants to merge 4 commits into from

Conversation

clionachee
Copy link

Motivation

The motivation behind this PR is to address issue #5963, which was opened on Sep 18, 2023, by nflaig. The problem described in the issue is that the usage of "*" as a value in certain CLI flags is not friendly to use in bash scripts. This is because * is interpreted as a wildcard character in bash, rather than as a string. The goal of this PR is to replace the usage of "*" with a more bash-friendly value, such as the word "all", to improve compatibility with bash scripts.

Description

The changes made in this PR involve replacing the usage of "*" with "all" in the enabledAll variable declaration. This change ensures better compatibility with bash scripts, as "*" can cause issues when interpreted as a wildcard character in bash environments. The modification is made in the api.ts file located at lodestar/packages/cli/src/options/beaconNodeOptions/api.ts, specifically on line 4.

Closes #5963

Steps to test or reproduce

@clionachee clionachee requested a review from a team as a code owner February 20, 2024 17:06
@CLAassistant
Copy link

CLAassistant commented Feb 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Thanks @clionachee for looking into it.

Please make sure to keep it backward compatible by still supporting "*" as a valid input to enable all namespaces.

Also should make sure to update other references in the code

packages/cli/src/options/beaconNodeOptions/api.ts Outdated Show resolved Hide resolved
packages/cli/src/options/beaconNodeOptions/api.ts Outdated Show resolved Hide resolved
Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

Thank you very much for your submission @clionachee !!!

If you would be so kind, would you mind added a simple comment to the relevant cors flags about needing to quote *?

Please make sure that we are backwards compatible (non-breaking change) so we support existing users. Thanks!!!

Looks like me and @nflaig both were reviewing concurrently and made the same comments. Great minds think alike lol

packages/cli/src/options/beaconNodeOptions/api.ts Outdated Show resolved Hide resolved
packages/cli/src/options/beaconNodeOptions/api.ts Outdated Show resolved Hide resolved
clionachee and others added 3 commits February 26, 2024 00:11
commit

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
@clionachee
Copy link
Author

I've committed the suggestions, are things going well, and thanks for the details in the comments for real. If I overlooked something, please let me know as well. Thanks again.

@nflaig
Copy link
Member

nflaig commented Feb 25, 2024

Thanks for the updates @clionachee.

We still need to make sure to

  • be backward compatible, i.e. if a user passes "*" it should enable all namespaces
  • and update namespace references in the code that still pass "*" and replace it with "all"

@nflaig
Copy link
Member

nflaig commented Mar 14, 2024

@clionachee are you still working on this? Let us know if you need further guidance on how to proceed

@clionachee
Copy link
Author

clionachee commented Mar 19, 2024

@clionachee are you still working on this? Let us know if you need further guidance on how to proceed

This is probably a rather dumb question, but about these, may I ask where do I get to change them, because I've committed the change. Sorry for any disturbance and thanks a bunch for real.

be backward compatible, i.e. if a user passes "" it should enable all namespaces
and update namespace references in the code that still pass "
" and replace it with "all"

@nflaig
Copy link
Member

nflaig commented Mar 23, 2024

This is probably a rather dumb question, but about these, may I ask where do I get to change them, because I've committed the change. Sorry for any disturbance and thanks a bunch for real.

You can just search the code base for "rest.namespace" to see where it is used. And to add backward compatibility is a simple change in the file you already edited.

You can simply test your changes by using the Dev CLI command to start Lodestar and try different paramets for --rest.namespace

@clionachee
Copy link
Author

clionachee commented Mar 25, 2024

Hi again, @nflaig I searched for rest.namespace , "rest.namespace": "*" and * but it said 'no matches found', so are there any specific string or variations being used? Sorry and thank you very much. If I cause too much of a trouble, please tell me or ignore me as well because I intend for good and really hope not to complicate things or kept on disturbing actually. Much love.

@matthewkeil
Copy link
Member

matthewkeil commented Mar 25, 2024

Thank you very much for you help and the attempt to fix this @clionachee! We really love our community and appreciate your attempt to fix this for us. If you are still having trouble despite the help from @nflaig I think this may be beyond your current skill level with TypeScript and the IDE. Please feel free to do a bit more digging and learning and come back and see us when you are a bit more comfortable with the development workflow. We will be here waiting for you and will be happy to help when you get some more experience under your belt. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace * value with other more bash friendly value
4 participants