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

[#5985] improvement(CLI): Fix role command that supports handling multiple values #5988

Merged
merged 4 commits into from
Dec 28, 2024

Conversation

Abyss-lord
Copy link
Contributor

@Abyss-lord Abyss-lord commented Dec 25, 2024

What changes were proposed in this pull request?

In GravitinoOptions, the role option supports multiple values, but the handleRoleCommand implementation currently only processes a single role value. CLI should support create and delete multiple roles simultaneously.

Why are the changes needed?

Fix: #5985

Does this PR introduce any user-facing change?

NO

How was this patch tested?

local test + UT

gcli role create -m demo_metalake --role role1 role2 role3
# role1, role2, role3 created

gcli role delete -m demo_metalake --role role1 role2 role3
# role1, role2, role3 deleted.

gcli role delete -m demo_metalake --role role1 role2 role3 unknown
# role1, role2, role3 deleted, but unknown is not deleted.

gcli role details -m demo_metalake
# Missing --role option.

gcli role details -m demo_metalake --role roleA roleB
# Exception in thread "main" java.lang.IllegalArgumentException: details requires only one role, but multiple are currently passed.

…ng multiple values

Fix role command that supports handling multiple values, CLI can create and delete multiple roles simultaneously.
@xunliu
Copy link
Member

xunliu commented Dec 26, 2024

@Abyss-lord Please fix the conflicting file. Thanks.

@Abyss-lord
Copy link
Contributor Author

@justinmclean Should GRANT and revoke also support multiple values?

@justinmclean
Copy link
Member

Probably not. You might want to grant multiple permissions at once (which is supported), but it's perhaps not that common to grant or revoke to/from multiple roles at once.

@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Dec 26, 2024

Probably not. You might want to grant multiple permissions at once (which is supported), but it's perhaps not that common to grant or revoke to/from multiple roles at once.

thank you Justin, only the create and delete commands support multiple values

@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Dec 26, 2024

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

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 for your contribution

@justinmclean justinmclean merged commit f6e201b into apache:main Dec 28, 2024
25 checks passed
@Abyss-lord Abyss-lord deleted the fix-role branch December 29, 2024 02:20
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.

[Improvement] The role command supports handling multiple values simultaneously.
4 participants