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/flag]: validation is skipping empty flag value #3003

Closed
Bidon15 opened this issue Dec 12, 2023 · 5 comments · Fixed by #3004
Closed

[cmd/flag]: validation is skipping empty flag value #3003

Bidon15 opened this issue Dec 12, 2023 · 5 comments · Fixed by #3004
Labels
bug Something isn't working external Issues created by non node team members good first issue Good for newcomers

Comments

@Bidon15
Copy link
Member

Bidon15 commented Dec 12, 2023

Celestia Node version

v0.12.1

OS

linux

Install tools

No response

Others

No response

Steps to reproduce it

celestia light start --p2p.network= --node.store=/home/celestia/.celestia-light-mocha-4 --core.ip=full.consensus.mocha-4.celestia-mocha.com --gateway --da.grpc.namespace=000008e5f679bf7116cb --da.grpc.listen=0.0.0.0:26650

Expected result

start fails with error that p2p.network is failed due to empty value provided

Actual result

celestia mainnet is started

Relevant log output

No response

Notes

No response

@Bidon15 Bidon15 added the bug Something isn't working label Dec 12, 2023
@github-actions github-actions bot added the external Issues created by non node team members label Dec 12, 2023
@Wondertan
Copy link
Member

start --p2p.network= --node.

The empty network is actually provided. Doesn't look like a problem.

@Bidon15
Copy link
Member Author

Bidon15 commented Dec 12, 2023

this is a UX problem that when a user is providing emptiness, node needs to reject such behaviours and ask for a specific value instead of accepting emptiness.

Emptiness is fine when there is no flag at all.

The current bug displays a UX problem when a user expects the light node to join mocha testnet, but ended up in celestia mainnet

@Wondertan
Copy link
Member

Wondertan commented Dec 12, 2023

On the first read, I misunderstood the issue due to reading the following as Actual Result, rather than Expected, and I thought you wanted to make it work with empty string.

start fails with error that p2p.network is failed due to empty value provided

In fact that's the opposite so the issue makes sense

@sontrinh16
Copy link
Contributor

hmmm i believe because of this line. We can return an error here instead if we want to reject it

@00x-dx
Copy link
Contributor

00x-dx commented Dec 13, 2023

I have pushed a fix at #3004. Ideally a shorthand network notation is good to have.

ramin added a commit that referenced this issue Jan 2, 2024
Fixes #3003. 
<!--
Thank you for submitting a pull request!

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

Please ensure you've addressed or included references to any related
issues.

Tips:
- Use keywords like "closes" or "fixes" followed by an issue number to
automatically close related issues when the PR is merged (e.g., "closes
#123" or "fixes #123").
- Describe the changes made in the PR.
- Ensure the PR has one of the required tags (kind:fix, kind:misc,
kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci,
kind:chore, kind:testing)

-->

---------

Co-authored-by: ramin <raminkeene@gmail.com>
renaynay pushed a commit to renaynay/celestia-node that referenced this issue Jan 15, 2024
Fixes celestiaorg#3003. 
<!--
Thank you for submitting a pull request!

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

Please ensure you've addressed or included references to any related
issues.

Tips:
- Use keywords like "closes" or "fixes" followed by an issue number to
automatically close related issues when the PR is merged (e.g., "closes
#123" or "fixes #123").
- Describe the changes made in the PR.
- Ensure the PR has one of the required tags (kind:fix, kind:misc,
kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci,
kind:chore, kind:testing)

-->

---------

Co-authored-by: ramin <raminkeene@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working external Issues created by non node team members good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants