Skip to content

Conversation

@clint-truss
Copy link
Contributor

Changes proposed in this pull request:

  • Makes it more clear how to provide a variable to override configuration of aborted, incomplete upload lifecycle rule
  • Ensures that rule ^ has all the proper defaults and doesn't throw warnings drift with the provider upgrade
  • Allows the analytics and inventory expiration days to be configured by the user as well as to enable/disable those rules if those elements are enabled/disabled rather than always being enabled.
  • Made the minimum provider version 5.89 now, and will bump the module version.

@clint-truss clint-truss requested a review from esacteksab March 17, 2025 23:14
@clint-truss
Copy link
Contributor Author

@esacteksab It's my first time bumping a module version. Do I just set a tag on the commit once this lands? Or is there more to it?

@clint-truss
Copy link
Contributor Author

So, when applying this, it goes cleanly and has no issues. When upgrading a bucket from the prior version of the module, you still get the drift because the defaults are changing, even though it's not materially different :-(

Given that you still get the drift, I wonder if there is any point in making this update at all? Because you could argue then that the original code wasn't technically broken 🤷

Here's what the drift looks like, which is identical to the drift that was there from before I updated the module:

  # module.settings_bucket.aws_s3_bucket_lifecycle_configuration.private_bucket will be updated in-place
  ~ resource "aws_s3_bucket_lifecycle_configuration" "private_bucket" {
        id                                     = "eclkc-stage-bootstrap-files"
      ~ transition_default_minimum_object_size = "varies_by_storage_class" -> "all_storage_classes_128K"
        # (2 unchanged attributes hidden)

      ~ rule {
            id     = "abort-incomplete-multipart-upload"
            # (2 unchanged attributes hidden)

          - filter {
                # (1 unchanged attribute hidden)
            }

          ~ noncurrent_version_expiration {
              + newer_noncurrent_versions = (known after apply)
                # (1 unchanged attribute hidden)
            }

          - noncurrent_version_transition {
              - noncurrent_days = 30 -> null
              - storage_class   = "STANDARD_IA" -> null
            }
          + noncurrent_version_transition {
              + newer_noncurrent_versions = (known after apply)
              + noncurrent_days           = 30
              + storage_class             = "STANDARD_IA"
            }

            # (2 unchanged blocks hidden)
        }
      ~ rule {
            id     = "aws-bucket-inventory"
          ~ status = "Enabled" -> "Disabled"
            # (1 unchanged attribute hidden)

          ~ expiration {
              ~ expired_object_delete_marker = false -> (known after apply)
                # (1 unchanged attribute hidden)
            }

          ~ filter {
              + object_size_greater_than = (known after apply)
              + object_size_less_than    = (known after apply)
                # (1 unchanged attribute hidden)
            }
        }
      ~ rule {
            id     = "aws-bucket-analytics"
            # (2 unchanged attributes hidden)

          ~ expiration {
              ~ expired_object_delete_marker = false -> (known after apply)
                # (1 unchanged attribute hidden)
            }

          ~ filter {
              + object_size_greater_than = (known after apply)
              + object_size_less_than    = (known after apply)
                # (1 unchanged attribute hidden)
            }
        }
    }

I think my changes help make it a bit clearer what the module is doing, but...I don't know if this is worth a major version upgrade now that it doesn't materially change anything. :-(

Copy link
Contributor

@esacteksab esacteksab left a comment

Choose a reason for hiding this comment

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

I ❤️ everything about this change, but the behavior you describe is disheartening. I agree with you, they're good changes, but maybe the major version change doesn't make sense.

I'll let you decide how to proceed, but if you want to cut a release, regardless of the version, I use 'releases' tab here. Happy to walk you through it some time if you'd like. It might have once been documented, but not certain where that would exist now and we do it so infrequently...

@clint-truss clint-truss merged commit 5493aa3 into main Mar 18, 2025
1 check passed
@clint-truss clint-truss deleted the ct-lifecycle-defaults branch March 18, 2025 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants