Skip to content

Fix flickering when transit gateway description is not set #32

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattock
Copy link

@mattock mattock commented Jan 17, 2023

what

This PR fixes flickering on every Terraform run when var.transit_gateway_description is unset.

The problem is caused by the leading space in the description: something that AWS APIs accept without erroring out or warning, but which gets removed or ignored. Hence Terraform tries to "fix" the situation on every run.

why

Without this change this happens on every Terraform run if var.transit_gateway_description is not defined:

Terraform will perform the following actions:                                                             

  # module.transit_gateway.aws_ec2_transit_gateway.default[0] will be updated in-place                    
  ~ resource "aws_ec2_transit_gateway" "default" {   
      ~ description                     = "Transit Gateway" -> " Transit Gateway"                         
        id                              = "tgw-07987d7d0a101968b"                                         
        tags                            = {}         
        # (11 unchanged attributes hidden)           
    }                                                

Plan: 0 to add, 1 to change, 0 to destroy.           

With this change applied things work as expected:

No changes. Your infrastructure matches the configuration.

Without this fix every Terraform run shows this change:
~ description = "Transit Gateway" -> " Transit Gateway"
@mattock mattock requested review from a team as code owners January 17, 2023 12:38
@nitrocode
Copy link
Member

The solution is to provide at least one of the context null label inputs such as name

A better fix for people who do not want to use name would be to use a join(" ", compact([module.this.id, "Transit Gateway"])). The description should probably be its own exposed input with the same default value Transit Gateway as it is today

@@ -12,7 +12,7 @@ locals {

resource "aws_ec2_transit_gateway" "default" {
count = module.this.enabled && var.create_transit_gateway ? 1 : 0
description = var.transit_gateway_description == "" ? format("%s Transit Gateway", module.this.id) : var.transit_gateway_description
description = var.transit_gateway_description == "" ? format("%sTransit Gateway", module.this.id) : var.transit_gateway_description
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = var.transit_gateway_description == "" ? format("%sTransit Gateway", module.this.id) : var.transit_gateway_description
description = var.transit_gateway_description == "" ? join(" ", compact([module.this.id, "Transit Gateway"])) : var.transit_gateway_description

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@mattock Please accept the suggested change. While we understand your solution, it degrades the most common approach, which is to use the null-label inputs and combine those with the " Transit Gateway" description. @nitrocode's solution solves for both and is preferred. Once the suggested change is accepted we'll get this approved + merged.

@Gowiem Gowiem requested review from Gowiem and removed request for RothAndrew February 20, 2024 20:21
@hans-d hans-d added the stale This PR has gone stale label Mar 7, 2024
Copy link

mergify bot commented Mar 9, 2024

Thanks @mattock 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 triage Needs triage and removed stale This PR has gone stale labels Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants