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

Manila: add share acl resource #526

Merged

Conversation

kayrus
Copy link
Collaborator

@kayrus kayrus commented Dec 14, 2018

Part of #502

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 14, 2018

Build failed.

@mrhillsman
Copy link

recheck

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 16, 2018

Build failed.

@kayrus kayrus force-pushed the manila-share-acl branch 2 times, most recently from 6ddc5fc to 2b5999a Compare December 17, 2018 12:09
@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 17, 2018

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 17, 2018

Build failed.

@kayrus
Copy link
Collaborator Author

kayrus commented Dec 20, 2018

I'm thinking about merging ACL into the share resource as a list of maps, like in aws security groups. Does it make sense?

@jtopjian
Copy link
Contributor

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 count.

aws_security_group looks very similar to openstack_compute_secgroup_v2 which caused a lot of limitations and problems. Thankfully, the Compute Security Group API became deprecated, so we could consider it a legacy resource and design openstack_networking_secgroup_v2 and openstack_networking_secgroup_rule_v2 appropriately.

It looks like an aws_security_group_rule resource also exists, probably to get around the same kinds of limitations. I have no idea how this resource and aws_security_group work together, though - I'd imagine a user must choose one method or another.

It's a best practice to create a decoupled/independent resource for any API call that can be decoupled.

@kayrus
Copy link
Collaborator Author

kayrus commented Dec 20, 2018

Thanks for the feedback.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 30, 2018

Build failed.

@kayrus kayrus force-pushed the manila-share-acl branch 2 times, most recently from 5ba4abe to e0e06d7 Compare December 30, 2018 19:06
@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 30, 2018

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 31, 2018

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 31, 2018

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 2, 2019

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 2, 2019

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 2, 2019

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 2, 2019

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 2, 2019

Build succeeded.

@kayrus
Copy link
Collaborator Author

kayrus commented Jan 2, 2019

@jtopjian ready for review

Copy link
Contributor

@jtopjian jtopjian left a 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.


access, err := shares.ListAccessRights(sfsClient, shareId).Extract()
if err != nil {
return CheckDeleted(d, err, "share")
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@kayrus kayrus force-pushed the manila-share-acl branch 2 times, most recently from 372a7b3 to b5faa83 Compare January 4, 2019 05:28
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 4, 2019

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 4, 2019

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 4, 2019

Build succeeded.

Copy link
Contributor

@jtopjian jtopjian left a 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.

@jtopjian jtopjian merged commit 459c6b6 into terraform-provider-openstack:master Jan 5, 2019
@kayrus kayrus deleted the manila-share-acl branch January 5, 2019 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants