Skip to content
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

Multiple cleanups #242

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Multiple cleanups #242

merged 1 commit into from
Dec 9, 2024

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Dec 8, 2024

what

why

discussion: why partially revert #236?

PR #236 enhanced the random_pet that determines part of the name of the DB instances so that the name would change whenever the instances would need to be recreated. Unfortunately, as a side-effect, that causes all instances created with earlier versions of this module to be replaced.

Upon further investigation, it was determined that all the "keepers" added by the PR would also force the cluster to be replaced. Unlike replacing an instance, replacing the cluster results in data loss. Also, unless you change the name of the cluster, the module both before and after the PR would fail, because it would try to create a new cluster with the same name before destroying the old cluster.

We prefer this failure mode, requiring the user to explicitly destroy the cluster before creating the new one, because it puts the user on notice about the potential data loss. So given that the changes in the PR did not make something work that did not work before, and it did cause disruption, we reverted the change to the keepers.

@Nuru Nuru added bugfix Change that restores intended behavior minor New features that do not break anything labels Dec 8, 2024
@Nuru Nuru requested review from a team as code owners December 8, 2024 04:48
@Nuru Nuru requested review from hans-d and nitrocode December 8, 2024 04:48
Copy link

mergify bot commented Dec 8, 2024

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Dec 8, 2024
@Nuru
Copy link
Contributor Author

Nuru commented Dec 8, 2024

/terratest

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
@oycyc
Copy link
Contributor

oycyc commented Dec 8, 2024

Appreciate this! Thanks for maintaining this module @Nuru 🙌

Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

LGTM

@Nuru Nuru merged commit 22dc338 into main Dec 9, 2024
41 of 61 checks passed
@Nuru Nuru deleted the pr-cleanup branch December 9, 2024 18:32
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

These changes were released in v1.15.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Change that restores intended behavior minor New features that do not break anything
Projects
None yet
3 participants