Skip to content

Conversation

@pripplinger
Copy link

No description provided.

@pripplinger pripplinger requested a review from a team as a code owner January 22, 2026 19:00
Copy link
Member

@HartmannVolker HartmannVolker left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a couple of remarks regarding the naming and sensible defaults.
To fix the pipeline you need to run terraform-docs markdown . --output-file=README.md --hide-empty in the module to update the README

Comment on lines 73 to 81
variable "lifecycle_days" {
description = "Lifespan of stored data. Data will be deleted after specified value in days. Default value is 0 (no automatic deletion)"
type = number
default = 0
validation {
condition = var.lifecycle_days >= 0
error_message = "The value for lifecycle in days must be 0 (deactivated) or a positive number."
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to to rename this to object_expiration_days. Additional the default should be null, it might be a valid use case to set the expiration to 0 to clean up large buckets.

Copy link
Author

Choose a reason for hiding this comment

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

Yes! Good idea!!
So we could ,e.g., create an expiration condition with a past date, if days is set to 0?

}

resource "aws_s3_bucket_lifecycle_configuration" "bucket_lifecycle" {
count = var.lifecycle_days > 0 ? 1 : 0
Copy link
Member

Choose a reason for hiding this comment

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

If the default it null this condition should be changed to `var.lifecycle_days != null ? 1 : 0

@pripplinger
Copy link
Author

Thanks for the Review! I overlooked the terraform-docs command in the Readme, thanks for pointing this out.

PR is updated

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