-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fix S3 bucket lifecycle rule filters with a single tag and no prefix #7162
Fix S3 bucket lifecycle rule filters with a single tag and no prefix #7162
Conversation
This change is not working in real situation using the terraform configuration from #7156 :( Read during plan phase:
Written during apply phase:
Re-read after apply phase:
Filter with single tag is still removed... |
Oh, this fix does work, I did not properly build the provider on my test machine in my first test :) Read during plan phase:
Written during apply phase:
Re-read after apply phase:
Note that this adds an empty prefix but is still interpreted the same in the web console. |
56bb1d6
to
ee4ae35
Compare
Re-tested with more debug logs. Read during plan phase:
Written during apply phase:
Re-read after apply phase:
|
@@ -755,7 +755,7 @@ func TestAccAWSS3Bucket_Logging(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccAWSS3Bucket_Lifecycle(t *testing.T) { | |||
func TestAccAWSS3Bucket_LifecycleBasic(t *testing.T) { |
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.
Note for reviewers: this allows to target this acceptance test without also matching the TestAccAWSS3Bucket_LifecycleExpireMarkerOnly
test.
ee4ae35
to
f237933
Compare
Also worth noting, this case is somewhat documented in the vendored AWS SDK:
|
Acceptance test results:
|
8c426be
to
c56cc2a
Compare
c56cc2a
to
b84c62e
Compare
Hi @bflad, would you mind having a look at this one? |
b84c62e
to
668bd49
Compare
668bd49
to
3cdf19a
Compare
Rebased on master. |
3cdf19a
to
786556f
Compare
786556f
to
ccc4aad
Compare
ccc4aad
to
b7f6ce0
Compare
Rebased on master. |
b7f6ce0
to
894704f
Compare
Rebased on master,
|
Rectification, they do pass:
|
I've been running the test in a loop for a while and it is quite flaky and fails from time to time:
|
Running the same test from master reveals similar issues (the test is named
It looks like the |
Also from master:
|
Here's a complete debug output when it is failing on master: https://gist.github.com/pdecat/8a74f9b2a8d3bc1fd075b747f1e15549 |
894704f
to
a59152e
Compare
a59152e
to
4f9273c
Compare
Rebased on master to resolve conflicts. |
9f0b8ae
to
64a012e
Compare
…sic to allow targetting this test without also matching TestAccAWSS3Bucket_LifecycleExpireMarkerOnly
64a012e
to
9a69443
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 @pdecat this looks good. It's such a subtle issue so thank you for calling out the edge case and going into detail within the original issue.
It is a bit of work to add an acceptance test that creates a lifecycle rule with a single tag and no prefix given our current implementation so it's okay to leave out. The updated test validates that the added logic works expected so that is a good step forward. I also validated that all other tests pass as expected prior to the approval. Thanks again.
Thanks @nywilken! |
This has been released in version 2.19.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 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! |
Fixes #7156
Changes proposed in this pull request:
Output from acceptance testing:
TODO:
Notes:
Rather than trying to add an acceptance test that creates a lifecycle rule with a single tag and no prefix the same way the web console would have done, I've settled to adapt the provider's update implementation to match the web console behavior.
Adding unit tests would probably be a good thing but this would require extracting lifecycle rules expanding/flattening code into functions and make the diff way less readable.