Skip to content

configurable bucket versioning #22

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

Merged
merged 1 commit into from
Apr 3, 2025
Merged

Conversation

jubrad
Copy link
Contributor

@jubrad jubrad commented Mar 3, 2025

This should help fix some deletion timeouts.

If you want to delete a bucket that has had versioning enabled the ideal way to do this is to set the version_ttl to one day, then wait a day, then delete. Haven't tested whether setting it to 0 would work.

@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from 4c93fc0 to 4129f8f Compare March 6, 2025 04:53
@jubrad jubrad requested a review from bobbyiliev March 7, 2025 05:42
@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from 4129f8f to cac84a4 Compare March 19, 2025 05:00
@jubrad jubrad marked this pull request as ready for review March 19, 2025 05:00
Copy link
Collaborator

@bobbyiliev bobbyiliev left a comment

Choose a reason for hiding this comment

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

Looks good!

@bobbyiliev bobbyiliev requested a review from kay-kim March 19, 2025 07:01
@bobbyiliev
Copy link
Collaborator

@jubrad shall we get that merged? There is a minor merge conflict but besides that I think that we are good to merge it and have it in the next release.

@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch 3 times, most recently from 5e04cad to 56cc638 Compare March 27, 2025 20:57
README.md Outdated
@@ -76,6 +76,8 @@ No resources.
| <a name="input_region"></a> [region](#input\_region) | The region where resources will be created | `string` | `"us-central1"` | no |
| <a name="input_use_local_chart"></a> [use\_local\_chart](#input\_use\_local\_chart) | Whether to use a local chart instead of one from a repository | `bool` | `false` | no |
| <a name="input_use_self_signed_cluster_issuer"></a> [use\_self\_signed\_cluster\_issuer](#input\_use\_self\_signed\_cluster\_issuer) | Whether to install and use a self-signed ClusterIssuer for TLS. Due to limitations in Terraform, this may not be enabled before the cert-manager CRDs are installed. | `bool` | `false` | no |
| <a name="input_storage_bucket_versioning"></a> [use\_storage\_bucket\_versioning](#input\_use\_storage\_bucket\_versioning) | Enable bucket versioning (Disabled by default due to deletion timeouts). | `bool` | `false` | no |
Copy link
Contributor

Choose a reason for hiding this comment

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

? We probably should regenerate the readme.md using terraform-doc? since I think it's just storage_bucket_versioning and storage_bucket_version_ttl instead of use_storage ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry I always forget about that.

README.md Outdated


#### Storage Bucket Versioning
By default storage bucket versinioning is turned off. This both reduces
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
By default storage bucket versinioning is turned off. This both reduces
By default, storage bucket versioning is turned off. This both reduces

Also, added MaterializeInc/materialize#32041 to go with this change. I just used v0.2.1 as a placeholder ... but can change to whatever version.

@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from 56cc638 to 2e5e5e8 Compare March 28, 2025 19:30
@@ -50,3 +57,10 @@ variable "lifecycle_rules" {
}
]
}

variable "version_ttl" {
description = "Sets the TTL (in days) on non current sotarge bucket objects."
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops - typo -> "storage"

variables.tf Outdated
}

variable "storage_bucket_version_ttl" {
description = "Sets the TTL (in days) on non current sotarge bucket objects."
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops - typo -> "storage"

variables.tf Outdated
@@ -158,6 +158,18 @@ variable "install_metrics_server" {
default = false
}

variable "storage_bucket_versioning" {
description = "Enable bucket versioning (Disabled by default due to deletion timeouts)."
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have "When enabled, set storage_bucket_version_ttl." or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just set a default of 7 days for versioning.

@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from 2e5e5e8 to c3ba56d Compare April 1, 2025 01:15
@jubrad jubrad changed the title disable bucket versioning by default configurable bucket versioning Apr 1, 2025
@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from c3ba56d to 04d9bfa Compare April 1, 2025 02:34
@jubrad jubrad force-pushed the disable-bucket-versioning-by-default branch from 04d9bfa to 00f9cfa Compare April 1, 2025 02:37
@jubrad jubrad merged commit d5ecda3 into main Apr 3, 2025
2 checks passed
@jubrad jubrad deleted the disable-bucket-versioning-by-default branch April 3, 2025 15:19
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