-
Notifications
You must be signed in to change notification settings - Fork 382
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
[#6123] fix(CLI): Refactor the validation logic of tag and role #6127
[#6123] fix(CLI): Refactor the validation logic of tag and role #6127
Conversation
Refactor the validation logic of the Role and fix the test case.
Refactor the validation logic of the Tag and fix the test case.
Hi @justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback. |
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
@@ -736,7 +724,7 @@ protected void handleTagCommand() { | |||
} | |||
|
|||
private String getOneTag(String[] tags) { | |||
if (tags.length > 1) { | |||
if (tags == null || tags.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is possible for tags to be null at this point, but I guess it doesn't hurt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks
…apache#6127) ### What changes were proposed in this pull request? Refactor the validation logic of the Tag and Role, meantime fix the test case. ### Why are the changes needed? Fix: apache#6123 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? ut + local test **Role test** ```bash gcli role grant -m demo_metalake --role admin # Missing --privilege option. gcli role revoke -m demo_metalake --role admin # Missing --privilege option. ``` **Tag test** ```bash gcli tag set -m demo_metalake # Missing --name option. gcli tag set -m demo_metalake --name catalog.schema.table --property property --tag tagA # Missing --value option. gcli tag set -m demo_metalake --name catalog.schema.table --value value --tag tagA # Missing --property option. gcli tag remove -m demo_metalake --tag tagA # Missing --name option. gcli tag remove -m demo_metalake # Missing --name option. gcli tag delete -m demo_metalake # Missing --tag option. gcli tag create -m demo_metalake # Missing --tag option. ```
What changes were proposed in this pull request?
Refactor the validation logic of the Tag and Role, meantime fix the test case.
Why are the changes needed?
Fix: #6123
Does this PR introduce any user-facing change?
No
How was this patch tested?
ut + local test
Role test
Tag test