Add support for setting bucket's logging config

[skinp]

Mostly self explanatory, this enables setting up which bucket to log access/storage logs to.

In theory, GCP also allows us to set a log_object_prefix. Unfortunately I couldn't easily get this working, so this is a first step. It's also why logging ends up being a TypeList instead of TypeString... to allow for future support of log_object_prefix.

Setting log_object_prefix wasn't obvious for the following reasons... In theory, this should an optional field but GCP sets it to the bucket name by default. It wasn't straightforward to go back and forth from an explicitly set value to the default because the expected TF state is "" but GCP reports it as the bucket name. It seems like the easiest approach would be to set a Default(Func) value for log_object_prefix dynamically to be the bucket's name, but I couldn't figure out how to make this work. Open for suggestions here...

[amoiseiev]

Added an issue for this pull request, for tracking purposes on our end: #972

[skinp]

@rosbo friendly bump?
I'm not sure we need to care about log_object_prefix immediately, it'd be useful for completeness, but just adding log_bucket support by itself is a step in the right direction I think (I just use log_bucket myself, not the prefix).

[danawillow]

Thanks @skinp, looks good! Just one quick nit.

Also, for log_object_prefix, setting the value as Computed would be a start (it would stop having diffs between empty and the default), but it does mean that if you set it to a custom value it won't show a diff if you try to set it back to the default. I'm generally in favor of having something that works most of the time now instead of something that works all the time later, but if you'd like to get something that works all the time check out some of the customdiff helpers (example from within our codebase here)

[danawillow]

this can just be else, right?

[skinp]

Yes, will fix

[skinp]

Right, I toyed with Computed for awhile but couldn't get it to revert back to the default if I removed the explicit config like you're describing. I'll go with the "something that works most of the time" for now.

[skinp]

Test run fwiw.

$ make testacc TEST=./google TESTARGS='-run=TestAccStorageBucket_' GOOGLE_USE_DEFAULT_CREDENTIALS=true
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccStorageBucket_ -timeout 120m
=== RUN   TestAccStorageBucket_basic
=== RUN   TestAccStorageBucket_lowercaseLocation
=== RUN   TestAccStorageBucket_customAttributes
=== RUN   TestAccStorageBucket_lifecycleRules
=== RUN   TestAccStorageBucket_storageClass
=== RUN   TestAccStorageBucket_update
=== RUN   TestAccStorageBucket_forceDestroy
=== RUN   TestAccStorageBucket_versioning
=== RUN   TestAccStorageBucket_logging
=== RUN   TestAccStorageBucket_cors
=== RUN   TestAccStorageBucket_labels
--- PASS: TestAccStorageBucket_lifecycleRules (4.15s)
--- PASS: TestAccStorageBucket_basic (4.17s)
--- PASS: TestAccStorageBucket_cors (2.91s)
--- PASS: TestAccStorageBucket_forceDestroy (15.21s)
--- PASS: TestAccStorageBucket_logging (16.47s)
--- PASS: TestAccStorageBucket_update (25.86s)
--- PASS: TestAccStorageBucket_versioning (12.97s)
--- PASS: TestAccStorageBucket_labels (24.35s)
--- PASS: TestAccStorageBucket_customAttributes (5.43s)
--- PASS: TestAccStorageBucket_lowercaseLocation (5.33s)
--- PASS: TestAccStorageBucket_storageClass (8.70s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	37.028s

[danawillow]

Thanks @skinp! Merging now.

[ghost]

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 👉 hashibot-feedback@hashicorp.com. Thanks!