-
Notifications
You must be signed in to change notification settings - Fork 386
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
[#5124][#5146] feat(auth-ranger): Ranger plugin should support rename operation #5176
Conversation
a248f11
to
d9466d1
Compare
api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java
Show resolved
Hide resolved
e54ee7c
to
bb20a1e
Compare
} | ||
|
||
/** | ||
* Remove the TABLE, Need to rename these the relevant policies, `*.{table}`, `*.{table}.{column}` |
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.
rename
-> delete
.
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.
DONE.
} | ||
|
||
/** | ||
* Remove the SCHEMA, Need to rename these the relevant policies, `{schema}`, `{schema}.*`, |
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.
rename
-> delete
?
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.
DONE.
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. some minors.
* @return return a metadataObject. | ||
*/ | ||
public MetadataObject getMetadataObject() { | ||
return this.metadataObject; |
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.
please remove this
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.
DONE.
* | ||
* @return return a metadataObject. | ||
*/ | ||
public MetadataObject getMetadataObject() { |
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.
remove get
from method name?
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.
DONE.
/** | ||
* IF remove the SCHEMA, Need to remove these the relevant policies, `{schema}`, `{schema}.*`, | ||
* `{schema}.*.*` <br> | ||
* IF remove the TABLE, Need to remove these the relevant policies, `{schema}.*`, `{schema}.*.*` |
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.
Need -> need
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.
DONE.
* | ||
* @return return a metadataObject object. | ||
*/ | ||
public MetadataObject getNewMetadataObject() { |
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.
remove get too?
* | ||
* @return return a metadataObject. | ||
*/ | ||
public MetadataObject getMetadataObject() { |
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.
get
LGTM except minor comments |
…pport rename operation (apache#5176) ### What changes were proposed in this pull request? Add rename a securable object in the RoleChange. ### Why are the changes needed? Fix: apache#5124 apache#5146 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Add ITs.
What changes were proposed in this pull request?
Add rename a securable object in the RoleChange.
Why are the changes needed?
Fix: #5124 #5146
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
Add ITs.