-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
acl: add default #28635
acl: add default #28635
Conversation
@@ -132,9 +132,9 @@ def _parse_acl(acl, user, group): | |||
|
|||
# If a user is not specified, use the owner of the file | |||
if comps[0] == 'user' and not comps[1]: | |||
comps[1] = user | |||
comps[1] = 'owner' |
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.
Could you please explain the reasoning behind 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.
the setfacl command can be used two ways:
- setfacl u:root:rw /tmp/house/kitchen
- setfacl u::rw /tmp/house/kitchen
then, there will be two {"user":[{"root": {"octal":6, "permissions":{"read": true,"write": true,"execute": false,}}}]}
if changing to 'owner',it will be {"user":[{"root": {"octal":6, "permissions":{"read": true,"write": true,"execute": false,}}}]} and {"user":[{"owner": {"octal":6, "permissions":{"read": true,"write": true,"execute": false,}}}]}
@thegoodduke There are a lot of changes here and it's not clear to me what the intention is. Could you please better explain the issue that this PR is intended to address? I'm not comfortable merging this until we have a better explanation of what's happening here and why these changes are necessary. Thanks. |
@thegoodduke This PR is failing a few of the ACL tests and also needs some lint errors cleaned up. Could you please have another look? Thanks. |
@thegoodduke Did you see my comment above? |
Go Go Jenkins! |
@thegoodduke We're going to have to close this if we don't get a response from the questions above. Could you please have a look? Thanks. |
my leader's pr has been merged(#29240), Thanks! |
add default supporting