provider/google: New Datasource: Google Storage Object Signed URL (supersedes #8180) #14643
Conversation
Implement content_md5, content_type, extension_headers support.
- extension_headers validation - header prefix must be ‘x-goog-‘ (with a trailing hyphen) - http_method validate, explicitly name the datasource attribute that is failing validation - remove redundant http_method validation that is no longer needed
|
@danawillow / @paddycarver if you could take a quick look, I think this is pretty straightforward. I don't know the details of these presigned urls though, but the included docs makes the reading of new/additional creds seem legit. It was reviewed back in Sept. as well, so I think this is good |
|
I'll take a look at this today/tomorrow, @catsby. I just want to re-read it again, and it's a bit long. :) |
|
Thanks @catsby - sorry I didn't circle back to this to rebase. |
|
@sl1pm4t no worries! Thanks for all the work you did. The rebase / conflict was just the layout, easy fix 😄 |
| func validateHttpMethod(v interface{}, k string) (ws []string, errs []error) { | ||
| value := v.(string) | ||
| value = strings.ToUpper(value) | ||
| if !regexp.MustCompile(`^(GET|HEAD|PUT|DELETE)$`).MatchString(value) { |
There was a problem hiding this comment.
nit: not crazy about using a regex to check if a string is one of four direct matches. Seems like a simple equality check would do here.
nit: not crazy about compiling the regexp on every run of the function, would rather use a global for that.
Neither of these should block merge.
| return errwrap.Wrapf("could not parse duration: {{err}}", err) | ||
| } | ||
| expires := time.Now().Unix() + int64(duration.Seconds()) | ||
| urlData.Expires = int(expires) |
There was a problem hiding this comment.
Not crazy about float64 -> int64 -> int, but that feels like a problem for not-today, seeing as this can still represent unix times correctly for the next 20-ish years.
| d.Get("bucket").(string), | ||
| d.Get("path").(string), | ||
| } | ||
| objectPath := strings.Join(path, "/") |
There was a problem hiding this comment.
nit: would rather do objectPath := "/"+strings.Join(path, "/") than rely on the empty-string-starts-the-array hack.
Shouldn't block merge.
| // 2. `credentials` attribute in the provider definition. | ||
| // 3. A JSON file whose path is specified by the | ||
| // GOOGLE_APPLICATION_CREDENTIALS environment variable. | ||
| func loadJwtConfig(d *schema.ResourceData, meta interface{}) (*jwt.Config, error) { |
There was a problem hiding this comment.
Eek. Another place where our credentials-loading algorithm is being defined. :( I don't see any way around this for the moment; we'll just have to clean up and sweep when credentials get an overhaul.
| Providers: testAccProviders, | ||
| Steps: []resource.TestStep{ | ||
| resource.TestStep{ | ||
| Config: testAccTestGoogleStorageObjectSingedUrl(bucketName), |
| // create HTTP request | ||
| url := a["signed_url"] | ||
| method := a["http_method"] | ||
| req, _ := http.NewRequest(method, url, nil) |
There was a problem hiding this comment.
Would really like to see this error handled.
Fix a typo, follow our acceptance test naming guidelines, simplify some logic, and handle an unhandled error.
|
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
locked and limited conversation to collaborators
I took #8180 and did the necessary rebase/merge. Thanks!