[#9377] feat(python-client): Add Role-related Interface. #9378
Conversation
Add Role-related Interface.
|
Hi @unknowntpo @tsungchih , could you please review this PR when you have time? I’d really appreciate your feedback. |
| class Name(Enum): | ||
| """The name of this privilege.""" | ||
|
|
||
| # The privilege to create a catalog. |
There was a problem hiding this comment.
I would recommend to place string literals directly below the variable name to serve as documentation that can be extracted by IDEs or documentation tools. For example:
CREATE_CATALOG = (0, 1 << 0)
"""The privilege to create a catalog."""| Returns: | ||
| str: A string representation of the AddSecurableObject instance. | ||
| """ | ||
| return f"ADDSECURABLEOBJECT {self._role_name} + {self._securable_object}" |
There was a problem hiding this comment.
I think there's no + in the string representation. It turns out to be f"ADDSECURABLEOBJECT {self._role_name} {self._securable_object}". Similar issue in other XXXSecurableObject.
| Returns: | ||
| SecurableObjectImpl: The created schema SecurableObject. | ||
| """ | ||
| names = catalog.full_name().split(".") |
There was a problem hiding this comment.
Why do we need to do split here? The behavior is different from the Java client.
| names.append(table) | ||
| return SecurableObjects.of( | ||
| MetadataObject.Type.TABLE, | ||
| names, |
There was a problem hiding this comment.
I would recommend to use unpacking here [*schema.full_name().split("."), table].
|
Hi @tsungchih , I've completed the code updates and would appreciate your review of the PR when you have a moment. Here's a summary of the commits:
|
tsungchih
left a comment
tsungchih
left a comment
There was a problem hiding this comment.
@jerryshao LGTM. Would you like to take further review.
|
@jerqi can you please help to review this, if there any missing pieces between Java and Python. |
| MANAGE_GRANTS = (0, 1 << 17) | ||
| """The privilege to grant or revoke a role for the user or the group.""" | ||
|
|
||
| CREATE_MODEL = (0, 1 << 18) |
There was a problem hiding this comment.
Could u use REGISTER_MODEL instead of CREATE_MODEL?
| CREATE_MODEL = (0, 1 << 18) | ||
| """The privilege to create a model.""" | ||
|
|
||
| CREATE_MODEL_VERSION = (0, 1 << 19) |
There was a problem hiding this comment.
Could u use LINK_MODEL_VERSION instead of CREATE_MODEL_VERSION?
| from gravitino.utils.precondition import Precondition | ||
|
|
||
|
|
||
| class RoleChange(ABC): |
There was a problem hiding this comment.
Actually, we didn't use RoleChange in the client side. So we could remove them.
| * @param securableObject The securable object. | ||
| * @param newSecurableObject The new securable object. | ||
| * @return return a RoleChange for the update securable object. | ||
| * @return return a RoleChange for the update-securable object. |
There was a problem hiding this comment.
Let us focus python part. Could u remove the modification?
4 participants
What changes were proposed in this pull request?
Add Role-related Interface.
Why are the changes needed?
Fix: #9377
Does this PR introduce any user-facing change?
no
How was this patch tested?
local test.