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.

Sign up for GitHub

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

Jump to bottom

[#6136] improvement(CLI): Move check for enable and disable command in Gravitino CLI to command #6535

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[#6136] improvement(CLI): Move check for enable and disable command in Gravitino CLI to command #6535

wants to merge 5 commits into from

Conversation

Abyss-lord
Copy link
Contributor

What changes were proposed in this pull request?

Move check for enable and disable command in Gravitino CLI to command

Why are the changes needed?

Fix: #6136

Does this PR introduce any user-facing change?

No

How was this patch tested?

local test

gcli metalake update -m demo_metalake --disable -i
demo_metalake has been disabled.

gcli metalake update -m demo_metalake --enable -i
demo_metalake has been enabled.

gcli metalake update -m demo_metalake --enable --disable -i
Unable to us --enable and --disable at the same time

 gcli metalake update -m demo_metalake --enable --all  -i
demo_metalake has been enabled. and all catalogs in this metalake have been enabled.

gcli catalog update -m demo_metalake --name Hive_catalog --disable -i
demo_metalake.Hive_catalog has been disabled.

gcli catalog update -m demo_metalake --name Hive_catalog --enable -i
demo_metalake.Hive_catalog has been enabled.

gcli catalog update -m demo_metalake --name Hive_catalog --enable --disable -i
Unable to us --enable and --disable at the same time

@Abyss-lord
[apache#6136] improvement(CLI): Move check for enable and disable com…
93a5ff3 
…mand in Gravitino CLI to command

move check for enable and disable command in ManageCatalog.
@Abyss-lord
[apache#6136] improvement(CLI): Move check for enable and disable com…
864177f 
…mand in Gravitino CLI to command

move check for enable and disable command in ManageMetalake.
@Abyss-lord
Copy link
Contributor Author

@justinmclean @tengqm could you please review this PR when you have time? I’d really appreciate your feedback.

justinmclean
justinmclean previously approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

tengqm
tengqm reviewed Feb 26, 2025
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageCatalog.java
@Override
public Command validate() {
if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) {
exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

The INVALID_ENABLE_DISABLE message text can be improved.

The --enable and the --disable options are mutual exclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tengqm Thank you , I just change the error message.

Copy link
Member

Choose a reason for hiding this comment

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

It should be "The --enable and the --disable options are mutually exclusive."

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm wondering if the reading level for that is a bit high "Unable to use --enable and --disable at the same time." is easier to understand. (Grade 9 vs Grade 2) The rest of the error messages are at a lower reading grade than Grade 9.

@Abyss-lord
[apache#6136] improvement(CLI): Move check for enable and disable com…
8fa11c0 
…mand in Gravitino CLI to command

fix some bugs.
@Abyss-lord
Copy link
Contributor Author

@justinmclean I’ve finished updating the code. Please take a look at the PR again when you have time.

@tengqm
Copy link
Contributor

tengqm commented Feb 26, 2025

/lgtm

@Abyss-lord
[apache#6136] improvement(CLI): Move check for enable and disable com…
d0e2c59 
…mand in Gravitino CLI to command

fix some bugs.
@Abyss-lord
Copy link
Contributor Author

@justinmclean I’ve finished updating the code. Please take a look at the PR again when you have time. I change the error message to "Unable to use --enable and --disable at the same time."

@tengqm
Copy link
Contributor

tengqm commented Feb 26, 2025

/lgtm again

justinmclean
justinmclean approved these changes Feb 27, 2025
View reviewed changes
Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

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

LGTM

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

@tengqm tengqm tengqm left review comments

@justinmclean justinmclean justinmclean approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Move check for enable and disable command in Gravitino CLI to command
3 participants
@Abyss-lord @tengqm @justinmclean