-
Notifications
You must be signed in to change notification settings - Fork 708
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
Add container app module #1792
Add container app module #1792
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @trapeznikov, Im running examples and getting:
│ Error: Reference to undeclared input variable
│
│ on module.tf line 106, in module "example":
│ 106: container_app = var.container_app
│
│ An input variable with the name "container_app" has not been declared. This variable can be declared with a variable
│ "container_app" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│
│ on module.tf line 107, in module "example":
│ 107: container_app_dapr_component = var.container_app_dapr_component
│
│ An input variable with the name "container_app_dapr_component" has not been declared. This variable can be declared with
│ a variable "container_app_dapr_component" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│
│ on module.tf line 108, in module "example":
│ 108: container_app_environment = var.container_app_environment
│
│ An input variable with the name "container_app_environment" has not been declared. This variable can be declared with a
│ variable "container_app_environment" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│
│ on module.tf line 109, in module "example":
│ 109: container_app_environment_certificate = var.container_app_environment_certificate
│
│ An input variable with the name "container_app_environment_certificate" has not been declared. This variable can be
│ declared with a variable "container_app_environment_certificate" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│
│ on module.tf line 110, in module "example":
│ 110: container_app_environment_storage = var.container_app_environment_storage
│
│ An input variable with the name "container_app_environment_storage" has not been declared. This variable can be declared
│ with a variable "container_app_environment_storage" {} block.
╵
Terraform plan return code: 1
Error 1 on or near line 57: Error running terraform plan; exiting with status 1
@arnaudlh need to add container_app to https://github.com/Azure/caf-terraform-landingzones/tree/main/caf_solution. What's the process of adding a new feature to these 2 repos? Should the change first go to |
|
@arnaudlh I fixed the variables file in the example module, it should run fine now. Sorry for the confusion, I was just testing it differently. |
I am currently working with this PR code. I hope it gets merged soon. Maybe not the right place but please follow hashicorp/terraform-provider-azurerm#21747 as workload profiles support is something in the future that should be supported by CAF as well. |
d9ec19b
to
bdbe498
Compare
@arnaudlh I resolved all the comment and rebased the branch. Can you please review? |
@arnaudlh @LaurentLesle can you please review this PR? |
log_analytics_workspace_id = can(var.settings.log_analytics_workspace_id) ? var.settings.log_analytics_workspace_id : var.diagnostics.log_analytics[var.settings.log_analytics_key].id | ||
infrastructure_subnet_id = try(var.subnet_id, null) | ||
internal_load_balancer_enabled = try(var.settings.internal_load_balancer_enabled, false) | ||
tags = merge(local.tags, try(var.settings.tags, null)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please help to add support for another 2 parameter: dapr_application_insights_connection_string & zone_redundancy_enabled. And I received error for internal_load_balancer_enabled for example 101 aca. but after I change false to null it works. I think for optional parameter we can just pass null if not specify like this:
internal_load_balancer_enabled = try(var.settings.internal_load_balancer_enabled, null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trapeznikov good PR - thank you! if you dont mind adding the 2 attributes mentionned by @iriahk89, I think we are good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trapeznikov I reviewed the PR and would really appreciate if you could add those 2 features and resolve the minor bug as one of my customers also waiting for this resource. Let me know if you need helps. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. we can add the integration with app insight combined object later for dapr_application_insights_connection_string if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks for your contribution @trapeznikov and for your reviews @iriahk89 @jvdhaterd |
1301
PR Checklist
Description
Does this introduce a breaking change
Testing