Add google_storage_default_object_acl resource #992
Conversation
| config := meta.(*Config) | ||
|
|
||
| bucket := d.Get("bucket").(string) | ||
| role_entity := make([]interface{}, 0) |
There was a problem hiding this comment.
nit: use mixedCaps please for variables (https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps)
There was a problem hiding this comment.
I'll fix the variable naming in the next review, once I get your thoughts on the comment above
Thanks!
| ForceNew: true, | ||
| }, | ||
|
|
||
| "role_entity": &schema.Schema{ |
There was a problem hiding this comment.
I'd highly recommend just having two separate fields here- one for the role, and the other for the entity. The id for the resource can then be bucket/entity, which would allow reading back the resource just from the id. If a Terraform user wants to specify multiple default acls, they can just have multiple resources. I'm going to hold off on reviewing the rest of this PR for now since it'll change a lot if you end up going with this approach.
There was a problem hiding this comment.
I don't necessarily think it is a good idea, because it is very likely that users will have multiple default acls. Default acls get applied to the objects only at object creation time, which means that the explicit dependency will need to be set for "google_storage_bucket_object" resource on "google_storage_default_object_acl" resource to ensure that if both of those resources are created simultaneously, the objects always gets created after the default acl is in place. Having multiple resources will make it more complicated to control for those explicit dependencies.
Besides, it seems to me that this resource should be similar in use with google_storage_object_acl, since it essentially does the same thing(sets up object permissions), but on the bucket level. So it seems to me that both should have similar resource declaration
There was a problem hiding this comment.
Ah ok, I can get behind having them be consistent with each other. That's fine then, I'll continue reviewing as-is :)
| } | ||
|
|
||
| func getDefaultObjectAclId(bucket string) string { | ||
| return bucket + "-default-object-acl" |
There was a problem hiding this comment.
That suffix isn't really necessary
| d.Set("role_entity", role_entity) | ||
| } | ||
|
|
||
| d.SetId(getDefaultObjectAclId(bucket)) |
There was a problem hiding this comment.
The id should be set at the end of create, since we want to make sure the state file has some information regardless of whether or not the read succeeded.
| role_entity = v.([]interface{}) | ||
| } | ||
|
|
||
| if len(role_entity) > 0 { |
There was a problem hiding this comment.
Let's do MinItems: 1 on the schema element for this and then remove this check
| bucket := d.Get("bucket").(string) | ||
| role_entity := make([]interface{}, 0) | ||
|
|
||
| if v, ok := d.GetOk("role_entity"); ok { |
There was a problem hiding this comment.
You can just do role_entity := d.Get("role_entity"), no need for the GetOk
|
|
||
| bucket := d.Get("bucket").(string) | ||
|
|
||
| if _, ok := d.GetOk("role_entity"); ok { |
| bucket := d.Get("bucket").(string) | ||
|
|
||
| if _, ok := d.GetOk("role_entity"); ok { | ||
| role_entity := make([]interface{}, 0) |
There was a problem hiding this comment.
nit: roleEntities since it's a list?
| Entity: pair.Entity, | ||
| } | ||
|
|
||
| // If the old state is missing for this entity, it needs to |
There was a problem hiding this comment.
nit: This comment is written as create -> update, but the if statement is update -> create, can it be reworded to match?
|
|
||
| bucket := d.Get("bucket").(string) | ||
|
|
||
| if d.HasChange("role_entity") { |
There was a problem hiding this comment.
Since role_entity is currently the only thing that can change, what do you think about:
if !d.HasChange("role_entity") {
return nil
}
and then the rest of the logic afterward?
| `, bucketName) | ||
| } | ||
|
|
||
| func testGoogleStorageDefaultObjectsAclBasic1(bucketName string) string { |
There was a problem hiding this comment.
These three tests are exactly the same except for which vars are filled in for the role_entity. How about something like this:
func testGoogleStorageDefaultObjectAclBasic(bucketName, roleEntity1, roleEntity2 string) string {
return fmt.Sprintf(`
// config here
`, bucketName, roleEntity1, roleEntity2)
|
|
||
| # google\_storage\_default\_object\_acl | ||
|
|
||
| Creates a new default object ACL in Google cloud storage service (GCS). For more information see |
There was a problem hiding this comment.
nit: Google Cloud Storage
Cleaning up storage_default_object_acl
|
Looks good! I just pushed a quick commit to fix up the docs so the link shows up in the sidebar. Merging now. |
* Storage Default Object ACL resource * Fixed the doc * Renamed the resource id. Log change * Complying with go vet * Changes for review * link to default object acl docs in sidebar
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
This addresses issue #64 and adds support for creating storage default object acls (API doc
The resource is similar to an existing resource for storage_object_acl, with the exception of setting up permissions on the bucket level