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

Better Front Door and Private Link integration due to module outputs #20

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

icklsede
Copy link
Contributor

@icklsede icklsede commented Aug 24, 2023

Describe your changes

  • add default_domain to outputs to determine internal loadbalancer IP for Private Link Service integration

  • rename container_app_fqdn to container_app_uri and make container_app_fqdn a real FQDN The actual fqdn is in fact a URI, which can't be used as Front Door Origin, as it requires a bare FQDN.

Issue number

#21

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Thanks for your cooperation!

icklsede and others added 2 commits August 24, 2023 18:01

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* add default_domain to outputs to determine internal
  loadbalancer IP for Private Link Service integration

* rename container_app_fqdn to container_app_uri and make
  container_app_fqdn a real FQDN
  The actual fqdn is in fact a URI, which can't be used as
  Front Door Origin, as it requires a bare FQDN.
@icklsede icklsede marked this pull request as ready for review September 8, 2023 08:51
@icklsede
Copy link
Contributor Author

icklsede commented Sep 8, 2023

Unfortunately the PR-check failed because it is a module with a "dubious ownership" being in use :)

│ Error: Failed to download module
│
│   on main.tf line 27:
│   27: module "public_ip" {
│
│ Could not download module "public_ip" (main.tf:27) source code from
│ "git::https://github.com/lonegunmanb/terraform-lonegunmanb-public-ip?ref=v0.1.0":
│ error downloading
│ 'https://github.com/lonegunmanb/terraform-lonegunmanb-public-ip?ref=v0.1.0':
│ /usr/bin/git exited with 128: fatal: detected dubious ownership in
│ repository at '/src/examples/acr/.terraform/modules/public_ip'
│ To add an exception for this directory, call:
│
│       git config --global --add safe.directory
│ /src/examples/acr/.terraform/modules/public_ip
│ .

@lonegunmanb
Copy link
Member

lonegunmanb commented Sep 11, 2023

Unfortunately the PR-check failed because it is a module with a "dubious ownership" being in use :)

│ Error: Failed to download module
│
│   on main.tf line 27:
│   27: module "public_ip" {
│
│ Could not download module "public_ip" (main.tf:27) source code from
│ "git::https://github.com/lonegunmanb/terraform-lonegunmanb-public-ip?ref=v0.1.0":
│ error downloading
│ 'https://github.com/lonegunmanb/terraform-lonegunmanb-public-ip?ref=v0.1.0':
│ /usr/bin/git exited with 128: fatal: detected dubious ownership in
│ repository at '/src/examples/acr/.terraform/modules/public_ip'
│ To add an exception for this directory, call:
│
│       git config --global --add safe.directory
│ /src/examples/acr/.terraform/modules/public_ip
│ .

Thanks for your pr and apology for this inconvenience. I'll try to fix this issue, thanks for reporting it to us.


Terraform uses git to download Terraform module, so you saw this issue because the owner of '/src/examples/acr` is not the user you were using.

I'm thinking of adding git config --global --add safe.directory $(pwd) to the script, but I'm afraid maybe it is way to much for this issue.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @icklsede for opening this pr, some review comments.

@@ -5,10 +5,20 @@ output "container_app_environment_id" {

output "container_app_fqdn" {
description = "The FQDN of the Container App's ingress."
value = { for name, container in azurerm_container_app.container_app : name => "${try(container.ingress[0].fqdn, "")}" if can(container.ingress[0].fqdn) }
Copy link
Member

Choose a reason for hiding this comment

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

Hi @icklsede thanks for opening this pr! Would you please simplify this expression to:?

 value       = { for name, container in azurerm_container_app.container_app : name => try(container.ingress[0].fqdn, "") if can(container.ingress[0].fqdn) }

Copy link
Member

Choose a reason for hiding this comment

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

And since you've changed the module's output, would you please update this example code to use the new output so our e2e test won't be broken? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lonegunmanb
The expression is just a copy from the URI output, I removed only the "https://" part. It will get the ingress FQDN from each container app and put it into a map with index of the container app name.

I'll send an update for the tests via squash commit.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @icklsede for the update, we have a typo in this pr, would you please correct it and run pr-check check command listed in our readme file to verify the pr? Thanks!

@@ -1,3 +1,9 @@
output "app_url" {
value = module.container_apps.container_app_uri
}
ouput "app_fqdn" {
Copy link
Member

Choose a reason for hiding this comment

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

Should be output here.

@lonegunmanb lonegunmanb changed the base branch from main to e-20 November 18, 2024 04:01
@lonegunmanb lonegunmanb merged commit a4104e1 into Azure:e-20 Nov 18, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants