feat(terraform): adds ignore_default_action input variable #140
Conversation
|
Hello 👋 @Gowiem @joe-niland can you please review this PR ? ;) Regards |
|
/terratest |
|
@jbrt checking it out, but these types of changes are always painful as adding a whole other resource / conditional logic to manage that resource means multiple locations that future changes need to be made. We'll want to make sure we actually want to support this use-case within the Cloud Posse ecosystem. I'll bring this up with the rest of the contributor team and I'll let you know. Side note -- you've got failures at the CI level. Mind doing the following locally and then pushing: Please run the following commands and commit the changes make init |
| @@ -171,8 +171,36 @@ resource "aws_lb_listener" "http_forward" { | |||
| } | |||
| } | |||
|
|
|||
| resource "aws_lb_listener" "http_forward_ignore_default_action" { | |||
There was a problem hiding this comment.
Huh this is beyond the scope of your PR, but noting for myself and any fellow contributors: I'm not even sure why we need to have two of these resources for http_forward + http_redirect. It seems we could just do the conditional logic In the default action itself without requiring two resources.
@joe-niland do you happen to know the history behind why it was done this way?
There was a problem hiding this comment.
@Gowiem sorry I missed this one! I don't know why but it dates back to the initial implementation I think. I agree it could be refactored as you described.
There was a problem hiding this comment.
@joe-niland I'll create an issue and we can see if a passer by wants to pick it up!
| @@ -40,23 +40,23 @@ output "default_target_group_arn_suffix" { | |||
|
|
|||
| output "http_listener_arn" { | |||
| description = "The ARN of the HTTP forwarding listener" | |||
| value = one(aws_lb_listener.http_forward[*].arn) | |||
| value = join("", aws_lb_listener.http_forward.*.arn, aws_lb_listener.http_forward_ignore_default_action.*.arn) | |||
There was a problem hiding this comment.
@jbrt for all of these lint errors, you do want to use brackets instead of .*. as that is the preferred way of using that operator.
|
@jbrt test error is surrounding a failure to delete a bucket it looks like. Probably not due to your changes. I'd make the lint fixes, we'll discuss internally, and then I'll rerun tests and we'll see if a 2nd run does the trick. |
|
this feels to me like a shortcut to deal with drift which is not the purpose of this module, those businesses's needs should be outside of this root module. you can still use this module to create the LB without any listeners and in your wrapper you can add an ignore lifecycle or any other needs you have. |
|
@jamengual solid point regarding these listener rules being created inside the root module instead of in the child. I agree with that suggestion as the path forward instead of introducing this complexity here @jbrt. Does that make sense? If you feel otherwise, feel free to state your case, but I'd suggest you take Pepe's approach. |
|
@jbrt going to close this PR out following the discussion above. Appreciate your work on this PR, but since this can be handled in the root module for your case and we can avoid adding additional complexity to an any already complex module we've decided against this enhance. |
what
Adds a new
ignore_default_action_changesinput variables.why
Because when this module is used in a AWS CodeDeploy Blue/Green deployment context, CodeDeploy will manipulates target groups and this module will drift. By adding a lifecycle on this module, it's now possible to use CodeDeploy deployments with this module.
references
Cloudposse's TF modules used for testing:
https://registry.terraform.io/modules/cloudposse/ecs-alb-service-task/aws/latest
https://registry.terraform.io/modules/cloudposse/alb/aws/latest
https://registry.terraform.io/modules/cloudposse/code-deploy/aws/latest