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

Storage Default Object ACL resource #1

Merged
merged 4 commits into from
Jan 22, 2018
Merged

Conversation

ishashchuk
Copy link

This addresses issue #67

It's very similar to an existing resource for storage_object_acl, with the exception of setting up permissions on the bucket level. Link to API can be found in the DOC portion, but also can be quickly viewed here

Creates a new default object ACL in Google Cloud Storage.
---

# google\_storage\_object\_acl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be google_storage_default_object_acl.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

resource.TestStep{
Config: testGoogleStorageDefaultObjectsAclBasic1(bucketName),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageDefaultObjectAcl(bucketName, roleEntityBasic1),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to copy/paste the variables for roleEntity... into the test file?

Copy link
Author

@ishashchuk ishashchuk Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it on purpose: i'm using the roleEntity variable definitions from resource_storage_bucket_acl_test.go. This permissions set is fine to use for testing purposes for all ACL resources.
Besides, all the tests are passing:

make testacc TEST=./google TESTARGS='-run=TestAccGoogleStorageDefaultObjectAcl'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleStorageDefaultObjectAcl -timeout 120m
=== RUN   TestAccGoogleStorageDefaultObjectAcl_basic
=== RUN   TestAccGoogleStorageDefaultObjectAcl_upgrade
=== RUN   TestAccGoogleStorageDefaultObjectAcl_downgrade
--- PASS: TestAccGoogleStorageDefaultObjectAcl_basic (14.37s)
--- PASS: TestAccGoogleStorageDefaultObjectAcl_upgrade (24.78s)
--- PASS: TestAccGoogleStorageDefaultObjectAcl_downgrade (25.00s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 25.240s

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Sorry, I was forgetting that those variables would be exposed at the module level. When I didn't see them in your file, I got very confused. :)

Copy link

@amoiseiev amoiseiev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update the fork with the most recent changes from the original?

Also, could you please check / make sure logging statements use is consistent with bucket acls resource?

}

func getDefaultObjectAclId(bucket string) string {
return bucket + "-default-acl"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make it bucket + "-default-object-acl"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged upstream into the fork.

I tried to make it consistent with object_acl resource, where they ID object + "-acl" but since it's a bucket level change, I think you are right it will be, more staright forward if we add object in there. changing to "-default-object-acl".

Logging seems consistent to me. Added a small change

@ishashchuk
Copy link
Author

@amoiseiev, I merged upstream into our fork and my branch. Also confirmed the logging and made some minor changes

@amoiseiev amoiseiev merged commit 58c7e0e into master Jan 22, 2018
@amoiseiev
Copy link

@ishashchuk merged. Thank you. Let's try upstream now

@ishashchuk ishashchuk deleted the storage_default_acl branch January 26, 2018 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants