-
Notifications
You must be signed in to change notification settings - Fork 359
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
Manila: add share acl resource #526
Manila: add share acl resource #526
Conversation
Build failed.
|
recheck |
Build failed.
|
6ddc5fc
to
2b5999a
Compare
Build failed.
|
2b5999a
to
c03af0f
Compare
Build failed.
|
I'm thinking about merging ACL into the share resource as a list of maps, like in aws security groups. Does it make sense? |
What's the reasoning behind merging the two? In general, I would advise against it. If the ACL is part of the share resource, then a user will never be able to decouple an ACL from a share to do things such as use
It looks like an It's a best practice to create a decoupled/independent resource for any API call that can be decoupled. |
Thanks for the feedback. |
c03af0f
to
c9cc7f9
Compare
Build failed.
|
5ba4abe
to
e0e06d7
Compare
Build failed.
|
e0e06d7
to
55bd79e
Compare
Build failed.
|
55bd79e
to
446918e
Compare
Build failed.
|
446918e
to
ca0e94d
Compare
Build failed.
|
Build failed.
|
Build failed.
|
Build failed.
|
Build succeeded.
|
@jtopjian ready for review |
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.
@kayrus Thank you for this one, too! Let me know if you have any questions.
openstack/resource_openstack_sharedfilesystem_share_access_v2.go
Outdated
Show resolved
Hide resolved
openstack/resource_openstack_sharedfilesystem_share_access_v2.go
Outdated
Show resolved
Hide resolved
|
||
access, err := shares.ListAccessRights(sfsClient, shareId).Extract() | ||
if err != nil { | ||
return CheckDeleted(d, err, "share") |
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.
This is only checking for a 404 return code. Will this ever return a 404, even if no acls exist?
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.
No, it wont. But if there is no share, it will return 404 and ACLs should be removed since they are subobjects of the share object.
openstack/resource_openstack_sharedfilesystem_share_access_v2.go
Outdated
Show resolved
Hide resolved
openstack/resource_openstack_sharedfilesystem_share_access_v2.go
Outdated
Show resolved
Hide resolved
openstack/resource_openstack_sharedfilesystem_share_access_v2.go
Outdated
Show resolved
Hide resolved
372a7b3
to
b5faa83
Compare
Build failed.
|
b5faa83
to
20f3e62
Compare
Build succeeded.
|
20f3e62
to
7c8ad6b
Compare
Build succeeded.
|
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. I'm going to squash and merge this PR to save you a rebase if I would merge #525 first.
Part of #502