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.

Sign up for GitHub

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

Jump to bottom

provider/google: Add CORS support for google_storage_bucket. #14695

Merged
merged 5 commits into from
May 31, 2017
Merged

provider/google: Add CORS support for google_storage_bucket. #14695

merged 5 commits into from
May 31, 2017

Conversation

rileykarson
Copy link
Contributor

Add support for CORS configuration fields to google_storage_bucket.

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/19 16:45:10 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccStorage -timeout 120m
=== RUN   TestAccStorage_basic
--- PASS: TestAccStorage_basic (2.90s)
=== RUN   TestAccStorageCustomAttributes
--- PASS: TestAccStorageCustomAttributes (6.12s)
=== RUN   TestAccStorageStorageClass
--- PASS: TestAccStorageStorageClass (7.09s)
=== RUN   TestAccStorageBucketUpdate
--- PASS: TestAccStorageBucketUpdate (8.40s)
=== RUN   TestAccStorageBucketImport
--- PASS: TestAccStorageBucketImport (2.82s)
=== RUN   TestAccStorageForceDestroy
Created object bucketDestroyTestFile at location https://www.googleapis.com/storage/v1/b/tf-test-acl-bucket-4919795548258768973/o/bucketDestroyTestFile

--- PASS: TestAccStorageForceDestroy (14.62s)
=== RUN   TestAccStorage_cors
--- PASS: TestAccStorage_cors (2.78s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	44.852s

Fix for #12519.
@danawillow

@danawillow danawillow self-requested a review May 20, 2017 01:22
danawillow
danawillow suggested changes May 22, 2017
View reviewed changes
builtin/providers/google/resource_storage_bucket.go Outdated
@@ -89,6 +89,40 @@ func resourceStorageBucket() *schema.Resource {
},
},
},

"cors": &schema.Schema{
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this is a set but its contents are lists? (not a criticism, just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason really - List semantics weren't important to it, and the first similar block I looked at was a Set. Changed to List for consistency with the rest of the file; it also ends up simplifying a type assertion that way.

builtin/providers/google/resource_storage_bucket_test.go Outdated
return fmt.Sprintf(`
resource "google_storage_bucket" "bucket" {
name = "%s"
cors {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation

Also can you add another cors block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

website/source/docs/providers/google/r/storage_bucket.html.markdown Outdated

* `method` - (Optional) The list of HTTP methods on which to include CORS response headers, (GET, OPTIONS, POST, etc) Note: "*" is permitted in the list of methods, and means "any method".

* `response_header` - (Optional) The list of HTTP headers other than the [simple response headers](https://www.w3.org/TR/cors/#simple-response-header) to give permission for the user-agent to share across domains.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: too many spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

danawillow
danawillow reviewed May 25, 2017
View reviewed changes
builtin/providers/google/resource_storage_bucket.go Outdated
corsRules := make([]*storage.BucketCors, 0, len(configured))
for _, raw := range configured {
data := raw.(map[string]interface{})
corsRule := storage.BucketCors{}
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: totally just my opinion but I think filling out structs/maps inline looks a bit better:

		corsRule := storage.BucketCors{
			Origin:         convertSchemaArrayToStringArray(data["origin"].([]interface{})),
			Method:         convertSchemaArrayToStringArray(data["method"].([]interface{})),
			ResponseHeader: convertSchemaArrayToStringArray(data["response_header"].([]interface{})),
			MaxAgeSeconds:  int64(data["max_age_seconds"].(int)),
		}

and below:

		data := map[string]interface{}{
			"origin":          corsRule.Origin,
			"method":          corsRule.Method,
			"response_header": corsRule.ResponseHeader,
			"max_age_seconds": corsRule.MaxAgeSeconds,
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@danawillow danawillow merged commit 32d47b3 into hashicorp:master May 31, 2017
@danawillow danawillow mentioned this pull request Jun 6, 2017
Closed
@ghost
Copy link

ghost commented Apr 11, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danawillow danawillow danawillow approved these changes

Assignees
No one assigned
Labels
enhancement provider/google-cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rileykarson @danawillow @tombuildsstuff