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.

Sign up for GitHub

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

Jump to bottom

Fixes #134 - Does not create http_forward when aws_lb_target_group.default is disabled #149

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Fixes #134 - Does not create http_forward when aws_lb_target_group.default is disabled #149

merged 2 commits into from
Jan 21, 2025

Conversation

spazm
Copy link
Contributor

@spazm spazm commented Oct 25, 2023 •
edited
Loading

What

Fixes #134 - Does not create http_forward when aws_lb_target_group.default is disabled

  • disables aws_lb_listener.http_forward when default_target_group is not enabled
  • target_group is required when type is 'redirect'

Why

See #134. Fixes this Validation error when default_target_group_enabled == 0 :

module.alb.aws_lb_listener.http_forward[0]: Creating...
╷
│ Error: creating ELBv2 Listener
(arn:aws:elasticloadbalancing:...:...:loadbalancer/...):
ValidationError: A target group ARN must be specified
│       status code: 400, request id:
7cf9d727-fc77-4d32-a160-deadbeefcafe
│
│   with module.alb.aws_lb_listener.http_forward[0],
│   on .terraform/modules/alb/main.tf line 150, in resource
"aws_lb_listener" "http_forward":
│  150: resource "aws_lb_listener" "http_forward" {

references

closes #134

@spazm spazm requested review from a team as code owners October 25, 2023 14:13
@spazm spazm requested review from srhopkins and woz5999 and removed request for a team October 25, 2023 14:13
@spazm spazm mentioned this pull request Oct 25, 2023
Closed
@spazm spazm force-pushed the 134_disable_arn_when_default_target_group_disabled branch 2 times, most recently from 8a9eb28 to 8cb422d Compare October 25, 2023 14:27
@spazm spazm force-pushed the 134_disable_arn_when_default_target_group_disabled branch from 8cb422d to d1a69cb Compare November 2, 2023 06:03
@spazm spazm changed the title Fixes #134 - Provides guard against referencing disabled default aws_lb_target_group Fixes #134 - Does not create http_forward when aws_lb_target_group.default is disabled Nov 2, 2023
@spazm
Copy link
Contributor Author

spazm commented Nov 2, 2023

Edited to remove unnecessary guard rails for the other aws_lb_listener blocks; they work fine when the default target group is disabled.

This leaves a small, clear change. Please reply with any questions and feel welcome to reformat or cherry-pick as necessary.

@Gowiem
Copy link
Member

Gowiem commented Nov 15, 2023

/terratest

@Gowiem Gowiem self-requested a review November 15, 2023 02:11
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 7, 2024

@spazm Thanks. For the failing test, can you do

README.md is outdated. Please run the following commands locally and push the file:
  make init
  make readme

@hans-d hans-d added the stale This PR has gone stale label Mar 8, 2024
@mergify Mergify
Copy link

mergify bot commented Mar 9, 2024

Thanks @spazm 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 15, 2024
@Gowiem
Copy link
Member

Gowiem commented Jan 20, 2025

@spazm Unfortunately, we need to run some automation to pass our tests. Mind doing the following locally, adding + committing the result, and pushing to your branch?

make init
make readme

Thanks!

@Gowiem Gowiem added patch A minor, backward compatible change bugfix Change that restores intended behavior labels Jan 20, 2025
@Gowiem Gowiem self-assigned this Jan 20, 2025
@spazm
Copy link
Contributor Author

spazm commented Jan 20, 2025

@spazm Unfortunately, we need to run some automation to pass our tests. Mind doing the following locally, adding + committing the result, and pushing to your branch?

make init
make readme

Thanks!

I missed the original requests to do so. I'll add that to my todo list and push back the results.

spazm added 2 commits January 21, 2025 01:01
@spazm
Fixes cloudposse#134 - Skips http_forward when aws_lb_target_group.de…
31ed3e6 
…fault is disabled

* disables aws_lb_listener.http_forward when default_target_group is not enabled
* target_group is required when type is 'redirect'

Fixes this Validation error when default_target_group_enabled == 0 :

```
module.alb.aws_lb_listener.http_forward[0]: Creating...
╷
│ Error: creating ELBv2 Listener
(arn:aws:elasticloadbalancing:...:...:loadbalancer/...):
ValidationError: A target group ARN must be specified
│       status code: 400, request id:
7cf9d727-fc77-4d32-a160-cbd175e16e20
│
│   with module.alb.aws_lb_listener.http_forward[0],
│   on .terraform/modules/alb/main.tf line 150, in resource
"aws_lb_listener" "http_forward":
│  150: resource "aws_lb_listener" "http_forward" {

```
@spazm
regenerate docs with make init readme
a3d75da
@spazm spazm force-pushed the 134_disable_arn_when_default_target_group_disabled branch from b6f73d5 to a3d75da Compare January 21, 2025 01:20
@Gowiem
Copy link
Member

Gowiem commented Jan 21, 2025

/terratest

Gowiem
Gowiem approved these changes Jan 21, 2025
View reviewed changes
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.

Thanks for the fix! :shipit:

@Gowiem Gowiem merged commit 9bfceac into cloudposse:main Jan 21, 2025
15 checks passed
@mergify mergify bot removed the triage Needs triage label Jan 21, 2025
@spazm
Copy link
Contributor Author

spazm commented Jan 21, 2025

Fantastic. Thanks for merging.

I've enjoyed using cloudposse tf modules, glad I could share back.

@github-actions GitHub Actions
Copy link
Contributor

These changes were released in v2.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gowiem Gowiem Gowiem approved these changes

@srhopkins srhopkins Awaiting requested review from srhopkins srhopkins is a code owner automatically assigned from cloudposse/contributors

@woz5999 woz5999 Awaiting requested review from woz5999 woz5999 is a code owner automatically assigned from cloudposse/contributors

Assignees

@Gowiem Gowiem

Labels
bugfix Change that restores intended behavior patch A minor, backward compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A target group ARN must be specified when disabling the default target group
3 participants
@spazm @Gowiem @hans-d