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

fix: listener default action variable object block [HOTFIX REQUIRED!] #190

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

oycyc
Copy link
Contributor

@oycyc oycyc commented Nov 11, 2024

what

Removes the default object block and changes it to null, making it consistent with the other default actions. The original mindset of having it was to default to HTTP_301 since it's a required attribute, but it doesn't matter because it is not optional() in the Terraform.

why

As described by @mschfh

this adds a redirect by default, as the default for the variable is an object, not null.

terraform-aws-alb/main.tf

Lines 231 to 232 in cb8fa65

dynamic "redirect" {
for_each = var.listener_https_redirect != null ? [var.listener_https_redirect] : []

variable "listener_https_redirect" {
description = "Have the HTTPS listener return a redirect response for the default action."
# https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lb_listener#status_code-2
type = object({
host = optional(string)
path = optional(string)
port = optional(string)
protocol = optional(string)
query = optional(string)
status_code = string
})
default = {
host = null
path = null
port = null
protocol = null
query = null
status_code = "HTTP_301"
}
}

Plan:

      ~ default_action {
          ~ type             = "forward" -> "redirect"
            # (2 unchanged attributes hidden)

          + redirect {
              + host        = "#{host}"
              + path        = "/#{path}"
              + port        = "#{port}"
              + protocol    = "#{protocol}"
              + query       = "#{query}"
              + status_code = "HTTP_301"
            }
        }

Explicitly passing listener_https_redirect = null to the module does prevent this change, please update the default or adjust the for_each.

image

references

Comment referencing this issue
#187 (comment)

@oycyc oycyc requested review from a team as code owners November 11, 2024 23:19
@oycyc oycyc requested review from hans-d and gberenice November 11, 2024 23:19
@mergify mergify bot added the triage Needs triage label Nov 11, 2024
@gberenice
Copy link

/terratest

Copy link

@gberenice gberenice left a comment

Choose a reason for hiding this comment

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

Thanks @oycyc!

@gberenice gberenice merged commit ef2c73b into cloudposse:main Nov 12, 2024
27 checks passed
@mergify mergify bot removed the triage Needs triage label Nov 12, 2024
Copy link
Contributor Author

@oycyc oycyc left a comment

Choose a reason for hiding this comment

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

@gberenice how can we create a release from this? It is still on 2.0.0 at the latest.

@oycyc
Copy link
Contributor Author

oycyc commented Nov 12, 2024

Ah looks like the CI failed.
https://github.com/cloudposse/terraform-aws-alb/deployments/release
iScreen Shoter - 20241112075239187

@oycyc
Copy link
Contributor Author

oycyc commented Nov 12, 2024

Ugh but the error about README.md.gotmpl never changed within the past 6 months
https://github.com/cloudposse/.github/blob/main/README.md.gotmpl

Do we need someone on the Cloudposse team to look into this?

Slack https://sweetops.slack.com/archives/CUJPCP1K6/p1731416487743859

@gberenice
Copy link

@oycyc yeah, this is a weird one. Thanks for reporting this to CP's channel ❤️

Copy link
Contributor

These changes were released in v2.1.0.

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.

2 participants