fix: FQDN detection when using an existing ALB

[legoscia]

Description

In the definition of the local variable atlantis_url, currently attributes of module.alb are referenced within a coalesce function call. coalesce isn't short-circuiting, so even if var.atlantis.fqdn was provided, it won't be used in the case when an existing ALB is used: the reference to module.alb.route53_records["A"] will fail, and the error will be caught by the top-level try, leaving atlantis_url as https://.

In this change, the individual references to module.alb are wrapped in try calls, meaning that var.atlantis.fqdn will be respected if provided.

Motivation and Context

This change fixes this issue: #377
With this change, links in the "Checks" section of a Github pull request point to the correct URL.

Breaking Changes

This change can force replacement of the ECS task definition in the case where an existing ALB was used and var.atlantis.fqdn was specified (but ignored). If this is not desirable, this can be avoided by removing that variable from the configuration. I believe this is unlikely to happen.

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects

I added the fqdn variable to github-separate, and deployed it without this change. That resulted in ATLANTIS_ATLANTIS_URL being set to https://. Then I ran terraform apply with this change. That resulted in module.atlantis.module.ecs_service.aws_ecs_task_definition.this[0] being recreated and ATLANTIS_ATLANTIS_URL being set to the URL of the load balancer.

• I have executed pre-commit run -a on my pull request

[bryantbiggs]

I think this all can be re-written and simplified to:

atlantis_url = "https://${try(
    var.atlantis.fqdn,
    module.alb.route53_records["A"].fqdn,
    module.alb.dns_name,
    "",
  )}"

[legoscia]

I tried that, but I got this error:

╷
│ Error: Invalid template interpolation value
│ 
│   on ../../main.tf line 3, in locals:
│    3:   atlantis_url = "https://${try(
│    4:     var.atlantis.fqdn,
│    5:     module.alb.route53_records["A"].fqdn,
│    6:     module.alb.dns_name,
│    7:     ""
│    8:   )}"
│     ├────────────────
│     │ module.alb.dns_name is null
│     │ module.alb.route53_records is object with no attributes
│     │ var.atlantis is object with no attributes
│ 
│ The expression result is null. Cannot include a null value in a string template.
╵

I think the problem is that module.alb.dns_name is always defined, even if it is null. Therefore, it doesn't throw an error, and try returns null.

I tried wrapping the try call in a call to coalesce, but that didn't fix the error, since coalesce raises an error when the final result is an empty string.

So I think we need the call to coalesce to drop the load balancer DNS name if it is null, and we need the try outside the coalesce to handle the case when there is no valid name. I removed the try around module.alb.dns_name, as it doesn't seem to be needed.

[bryantbiggs]

hmm, ok. well we can at least save one try() for now with the following. I believe this should work if what you are proposing works:

atlantis_url = "https://${try(coalesce(
    try(var.atlantis.fqdn, module.alb.route53_records["A"].fqdn, null),
    module.alb.dns_name,
  ), "")}"

[legoscia]

That's a good point. Just updated the commit - seems to work!

[bryantbiggs]

Thank you!

[antonbabenko]

This PR is included in version 4.0.6 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.