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

fix: change flag "name" to "id" and add "all" flag #562

Merged
merged 1 commit into from
May 20, 2022

Conversation

24sama
Copy link
Contributor

@24sama 24sama commented May 20, 2022

Pre-Checklist

Note: please complete ALL items in the following checklist.

  • I have read through the CONTRIBUTING.md documentation.
  • My code has the necessary comments and documentation (if needed).
  • I have added relevant tests

Description

  • Change flag "name" to "id".
  • Add "all" flag to dtm show status command.

Related Issues

#557

New Behavior (screenshots if needed)

@24sama 24sama changed the title change flag "name" to "id" and add "all" flag fix: change flag "name" to "id" and add "all" flag May 20, 2022
@daniel-hutao
Copy link
Member

daniel-hutao commented May 20, 2022

@24sama Hello Leo Li, I tested this pr locally and found some error(Warn):

./dtm show status --all

2022-05-20 14:19:29 ⚠ [WARN]  Empty instance name. Maybe you forgot to add --id=INSTANCE_ID. The default value "default" will be used.

Here may be some improvement can be done.


A gentle reminder: please pay attention to the conventional commit messages rules: https://www.conventionalcommits.org/en/v1.0.0/

We also have a link for it in our CONTRIBUTING.md.


What if the user doesn't enter any flags? I think --all defaults to true will be more appropriate

@24sama
Copy link
Contributor Author

24sama commented May 20, 2022

Hi @daniel-hutao , thanks for your review.

What if the user doesn't enter any flags? I think --all defaults to true will be more appropriate

dtm show status -p xxxx 

It will return all results if --all defaults to true, which isn't our expected.
I think we only need to add a little logic. If the user doesn't enter any flags, we make the value of --all to true.

And I make some changes, PTAL.

Signed-off-by: 24sama <jacksama@foxmail.com>
@daniel-hutao
Copy link
Member

Hi @daniel-hutao , thanks for your review.

What if the user doesn't enter any flags? I think --all defaults to true will be more appropriate

dtm show status -p xxxx 

It will return all results if --all defaults to true, which isn't our expected. I think we only need to add a little logic. If the user doesn't enter any flags, we make the value of --all to true.

And I make some changes, PTAL.

Cool, your are right, the --all flag should be true only when no flags provided.

Copy link
Member

@daniel-hutao daniel-hutao left a comment

Choose a reason for hiding this comment

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

lgtm

@daniel-hutao daniel-hutao merged commit a6ecd2c into devstream-io:main May 20, 2022
@daniel-hutao daniel-hutao added the enhancement New feature or request label May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants