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

implement create_before_destroy on instances #192

Closed
wants to merge 9 commits into from

Conversation

finchr
Copy link
Contributor

@finchr finchr commented Feb 27, 2024

what

I implemented create_before_destroy on the aws_rds_cluster_instance default instances.

why

Making a change to any parameter that triggers a replace on a aws_rds_cluster_instance results in all instances being destroyed before attempting to create a new instance which causes an outage. This a faster (and safer) altenative to #191

references

This closes #190 and is an alternative to #191

@finchr finchr requested review from a team as code owners February 27, 2024 18:27
@kevcube
Copy link
Sponsor Contributor

kevcube commented Feb 28, 2024

/terratest

@frimik
Copy link
Contributor

frimik commented Feb 29, 2024

Is there an upgrade path for this? The random_pet name will require cluster destroy if upgrading the module.

@finchr
Copy link
Contributor Author

finchr commented Feb 29, 2024

Is there an upgrade path for this? The random_pet name will require cluster destroy if upgrading the module.

Hi @frimik this will not destroy the cluster, it will only replace the db instances.

@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 2, 2024

@finchr hi, can you follow up on the reported check errors? thanks

@hans-d
Copy link

hans-d commented Mar 8, 2024

/terratest

@finchr finchr force-pushed the finchr_create_before_destroy branch from 6408784 to 5d19e96 Compare March 8, 2024 16:30
@finchr
Copy link
Contributor Author

finchr commented Mar 8, 2024

@finchr hi, can you follow up on the reported check errors? thanks

Hi @hans-d sorry for the delay, I fixed the issue pushed the change.

@hans-d
Copy link

hans-d commented Mar 8, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 8, 2024

@Gowiem can you have a look content wise?

@hans-d
Copy link

hans-d commented Mar 8, 2024

TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: ╷
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ Error: Provider produced inconsistent result after apply
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ 
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ When applying changes to
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ module.rds_cluster_aurora_serverlessv2_postgres_13.random_pet.instance,
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ provider "provider[\"registry.terraform.io/hashicorp/random\"]" produced an
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ unexpected new value: .prefix: was cty.StringVal(""), but now null.
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ 
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ This is a bug in the provider, which should be reported in the provider's
TestExamplesServerlessV2PostgresDisabled 2024-03-08T17:32:33Z logger.go:66: │ own issue tracker.

@Gowiem
Copy link
Member

Gowiem commented Mar 8, 2024

@hans-d @finchr I'm going to call in @Nuru on this one... this is one of those types of changes we want to be very thoughtful about and I'm sure Jeremy will have opinions.

@Gowiem Gowiem requested a review from Nuru March 8, 2024 21:19
Copy link

mergify bot commented Mar 9, 2024

Thanks @finchr for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@mergify mergify bot added the triage Needs triage label Mar 9, 2024
Copy link

mergify bot commented Mar 9, 2024

This pull request now has conflicts. Could you fix it @finchr? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Mar 9, 2024
@finchr finchr force-pushed the finchr_create_before_destroy branch from 5d19e96 to fe55b00 Compare March 11, 2024 16:38
@finchr
Copy link
Contributor Author

finchr commented Mar 11, 2024

This pull request now has conflicts. Could you fix it @finchr? 🙏

Conflicts resolved!

@mergify mergify bot removed the conflict This PR has conflicts label Mar 11, 2024
finchr and others added 5 commits March 15, 2024 10:34
* add option for enabling global write forwarding

* Update Readme with enable_global_write_forwarding ref.

* Add enable_global_write_forwarding option.

* update readme

* fix readme

---------

Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label May 7, 2024
Copy link

mergify bot commented May 7, 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.

@finchr
Copy link
Contributor Author

finchr commented May 8, 2024

Hi @hans-d @Nuru , any update this?

Copy link

mergify bot commented May 18, 2024

💥 This pull request now has conflicts. Could you fix it @finchr? 🙏

@mergify mergify bot added the conflict This PR has conflicts label May 18, 2024
Copy link

mergify bot commented May 18, 2024

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot closed this May 18, 2024
@mergify mergify bot removed conflict This PR has conflicts needs-cloudposse Needs Cloud Posse assistance triage Needs triage labels May 18, 2024
@finchr
Copy link
Contributor Author

finchr commented May 19, 2024

Seriously? You sit on this months and then request that I fix merge conflicts and close on the same day?

@osterman
Copy link
Member

Sorry about that. I think we have an edge condition with the mergify rules that caused that to happen. Please click the button to reopen the PR and work with someone in #pr-reviews channel to get this merged.

@finchr
Copy link
Contributor Author

finchr commented May 20, 2024

Sorry about that. I think we have an edge condition with the mergify rules that caused that to happen. Please click the button to reopen the PR and work with someone in #pr-reviews channel to get this merged.

Hi @osterman I don't see a button or option to reopen the PR.

@osterman
Copy link
Member

Hrm... looks like it won't work to reopen.

image

@osterman
Copy link
Member

We'll need a net-new PR.

@finchr finchr mentioned this pull request May 21, 2024
@finchr
Copy link
Contributor Author

finchr commented May 21, 2024

OK, new PR is #213

Benbentwo added a commit that referenced this pull request Jun 7, 2024
* implement create_before_destroy on instances

* add random provider to versions.tf

* run terraform fmt

* run `make readme` to pass ci action

* fix random_pet in case of enabled == true

* don't create resource in enabled == false

---------

Co-authored-by: Benjamin Smith <ben.smith.developer@gmail.com>
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.

Implement rolling update for instances.
8 participants