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

azurerm_container_app_environment - Make log_analytics_workspace_id an optional argument. #22733

Merged
merged 6 commits into from Aug 9, 2023

Conversation

lonegunmanb
Copy link
Contributor

We found that log_analytics_workspace_id is not required for azurerm_container_app_environment anymore, this pr change it to an optional argument so our users could save cost. This pr should solve #20748.

The newly added e2e test passed on my side:

=== RUN   TestAccContainerAppEnvironment_withoutLogAnalyticsWorkspace
=== PAUSE TestAccContainerAppEnvironment_withoutLogAnalyticsWorkspace
=== CONT  TestAccContainerAppEnvironment_withoutLogAnalyticsWorkspace
--- PASS: TestAccContainerAppEnvironment_withoutLogAnalyticsWorkspace (670.33s)

@@ -25,7 +25,7 @@ func TestAccContainerAppEnvironment_basic(t *testing.T) {

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.basic(data),
Config: r.basic(data, true),
Copy link
Member

Choose a reason for hiding this comment

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

Instead up updating the config to take an argument can you remove the property from basic and add it to the complete test instead.

@@ -44,7 +44,7 @@ The following arguments are supported:

* `location` - (Required) Specifies the supported Azure location where the Container App Environment is to exist. Changing this forces a new resource to be created.

* `log_analytics_workspace_id` - (Required) The ID for the Log Analytics Workspace to link this Container Apps Managed Environment to. Changing this forces a new resource to be created.
* `log_analytics_workspace_id` - (Optional) The ID for the Log Analytics Workspace to link this Container Apps Managed Environment to. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be moved down to the section where we list the Optional properties.

@lonegunmanb
Copy link
Contributor Author

@stephybun Thanks for your suggestions, I've updated this pr, would you please give it another review? Thanks!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @lonegunmanb LGTM ⭐

@stephybun stephybun merged commit d00e29a into hashicorp:main Aug 9, 2023
23 checks passed
@github-actions github-actions bot added this to the v3.69.0 milestone Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_container_app_environment should not require a log_analytics_workspace_id
2 participants