-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Add support for metadata on s3 objects #1945
Add support for metadata on s3 objects #1945
Conversation
Not sure how the pipeline works here. How long before this will hit the release version? |
Not sure neither. One of our PR have been merged within minutes while another took months. We just cross our fingers and wait. |
We have some Hashicorp licenses/support @ Dow Jones. Let me poke them from that direction. |
Cool, thanks! |
c0e0ff6
to
d878a5f
Compare
This PR would be ready to merge. We have been using it for a while now. Added an acceptance test. make testacc TESTARGS='-run=TestAccAWSS3BucketObject_metadata'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSS3BucketObject_metadata -timeout 120m
? github.com/terraform-providers/terraform-provider-aws [no test files]
=== RUN TestAccAWSS3BucketObject_metadata
--- PASS: TestAccAWSS3BucketObject_metadata (28.45s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 28.461s |
f1fffa3
to
b78710a
Compare
fea83d3
to
1b6be88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jocgir 👋 Thanks for submitting this. Please see the below initial feedback and let us know if you have any questions or do not have time to implement the items.
@@ -77,6 +77,12 @@ func resourceAwsS3BucketObject() *schema.Resource { | |||
Optional: true, | |||
}, | |||
|
|||
"metadata": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the Elem
configuration for this new TypeMap
attribute?
Elem: schema.Schema{Type: schema.TypeString},
meta[key] = &valueStr | ||
} | ||
|
||
putInput.Metadata = meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above conversion logic can be removed with using the existing stringMapToPointers
helper function:
putInput.Metadata = meta | |
putInput.Metadata = stringMapToPointers(v.(map[string]interface{})) |
@@ -299,6 +315,15 @@ func resourceAwsS3BucketObjectRead(d *schema.ResourceData, meta interface{}) err | |||
d.Set("content_encoding", resp.ContentEncoding) | |||
d.Set("content_language", resp.ContentLanguage) | |||
d.Set("content_type", resp.ContentType) | |||
metadata := pointersMapToStringList(resp.Metadata) | |||
|
|||
// AWS Go SDK capitalizes metadata, this is a workaround. https://github.com/aws/aws-sdk-go/issues/445 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this context!
CheckDestroy: testAccCheckAWSS3BucketObjectDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
PreConfig: func() {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous PreConfig
can be removed 👍
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckAWSS3BucketObjectDestroy, | ||
Steps: []resource.TestStep{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the new attribute does not include ForceNew: true
, this test should include a second TestStep
that updates the metadata information.
Co-Authored-By: jocgir <[email protected]>
Co-Authored-By: jocgir <[email protected]>
Co-Authored-By: jocgir <[email protected]>
…etadata-on-s3-objects
Hi folks 👋 Since we haven't seen anything in awhile regarding the remaining feedback items, they were finished up in a followup commit. Adding the additional test steps showed that Output from acceptance testing:
|
Reference: #1945 (review) Output from acceptance testing: ``` --- PASS: TestAccAWSS3BucketObject_contentBase64 (30.69s) --- PASS: TestAccAWSS3BucketObject_content (31.18s) --- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (31.51s) --- PASS: TestAccAWSS3BucketObject_empty (31.68s) --- PASS: TestAccAWSS3BucketObject_sse (31.84s) --- PASS: TestAccAWSS3BucketObject_source (32.12s) --- PASS: TestAccAWSS3BucketObject_updates (50.09s) --- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (51.04s) --- PASS: TestAccAWSS3BucketObject_kms (54.27s) --- PASS: TestAccAWSS3BucketObject_metadata (69.32s) --- PASS: TestAccAWSS3BucketObject_acl (72.38s) --- PASS: TestAccAWSS3BucketObject_tags (89.73s) --- PASS: TestAccAWSS3BucketObject_tagsLeadingSlash (90.66s) --- PASS: TestAccAWSS3BucketObject_storageClass (107.45s) ```
Oh wow. Completely forgot about updating this PR for merge. Thanks for making the changes yourself and merging @bflad 🎉 ❤️ |
This has been released in version 2.21.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests, documentation updates, or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
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. Thanks! |
No description provided.