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

Output instance endpoints, add attributes to random_pet that force a new instance #236

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

finchr
Copy link
Contributor

@finchr finchr commented Nov 8, 2024

…new instance

what

why

  • I need the actual instance endpoints for the Datadog DMS integration, the default dashboards work better with the exact instance identifier.
  • Currently if any of these attributes change (db_subnet_group_name, engine) it will bypass the random_pet and attempt to create instances with the same identifier.

references

@finchr finchr requested review from a team as code owners November 8, 2024 21:03
@finchr finchr requested review from hans-d and nitrocode November 8, 2024 21:03
@mergify mergify bot added the triage Needs triage label Nov 8, 2024
@aknysh aknysh changed the title output instance endpoints, add attributes to random_pet that force a … Output instance endpoints, add attributes to random_pet that force a new instance Nov 8, 2024
@aknysh
Copy link
Member

aknysh commented Nov 8, 2024

/terratest

@aknysh aknysh added minor New features that do not break anything and removed triage Needs triage labels Nov 8, 2024
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @finchr

@aknysh aknysh merged commit 8c21136 into cloudposse:main Nov 8, 2024
77 checks passed
Copy link

github-actions bot commented Nov 9, 2024

These changes were released in v1.13.0.

@mihaiplesa
Copy link

Is this not a breaking change with potential of data loss, especially for clusters of size 1? In that case it should be described as backwards incompatible and have a conditional flag to prevent recreation/destroy of existing resources.

@finchr
Copy link
Contributor Author

finchr commented Nov 21, 2024

Is this not a breaking change with potential of data loss, especially for clusters of size 1? In that case it should be described as backwards incompatible and have a conditional flag to prevent recreation/destroy of existing resources.

Hi @mihaiplesa this is not a breaking change, it just covers a few missed attributes from a previous PR which might have caused problems if certain attributes were changed and also adds the instance endpoints to the output.

@oycyc
Copy link
Contributor

oycyc commented Dec 4, 2024

image
this forces a replacement even though the actual value of the identifier hasn't changed, but because the attribute changed (the source of the attribute), it's forcing a replacement 😬

@@ -336,13 +339,13 @@ module "rds_identifier" {
resource "aws_rds_cluster_instance" "default" {
count = local.cluster_instance_count
identifier = "${module.rds_identifier[0].id}-${count.index + 1}"
cluster_identifier = coalesce(join("", aws_rds_cluster.primary[*].id), join("", aws_rds_cluster.secondary[*].id))
cluster_identifier = random_pet.instance[0].keepers.cluster_identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

this is forcing a replacement in the resource "aws_rds_cluster_instance" "default" { now that these attribute values are changing (even if the end result value is the same). this would cause a recreate of the database instance... see example plan above

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants