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

Made the app_port property in the dapr block of the azurerm_container_app resource optional #20567

Merged
merged 9 commits into from
Apr 1, 2023

Conversation

paolosalvatori
Copy link
Contributor

@paolosalvatori paolosalvatori commented Feb 20, 2023

This pull request makes the app_port property in the dapr block of the azurerm_container_app resource should optional instead of required. As explained in the related issue #20534, you should be able to set the value of this property to null to create headless applications, like the pythonapp in the Tutorial: Deploy a Dapr application to Azure Container Apps with an Azure Resource Manager or Bicep template, with no ingress, hence with no app_port. This PR resolves #20534

@paolosalvatori paolosalvatori changed the title Made app_port property optional Made the app_port property in the dapr block of the azurerm_container_app resource optional Feb 20, 2023
@zioproto
Copy link
Contributor

Cc: @lonegunmanb

Copy link
Contributor

@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.

A suggestion on your pr @paolosalvatori

@paolosalvatori
Copy link
Contributor Author

paolosalvatori commented Feb 20, 2023

@lonegunmanb I successfully tested the change locally by deploying a container app with no app_port in the dapr block.

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 for this PR @paolosalvatori!

Could you add a test where app_port is omitted so that we can test this behaviour? I also suspect that changes will need to be made to the expand/flatten functions for this block to conditionally set this property if it's been specified.

func ExpandContainerAppDapr(input []Dapr) *containerapps.Dapr {
if len(input) == 0 {
return nil
}
dapr := input[0]
if dapr.AppId == "" {
return &containerapps.Dapr{
Enabled: pointer.To(false),
}
}
appProtocol := containerapps.AppProtocol(dapr.AppProtocol)
return &containerapps.Dapr{
AppId: pointer.To(dapr.AppId),
AppPort: pointer.To(int64(dapr.AppPort)),
AppProtocol: &appProtocol,
Enabled: pointer.To(true),
}
}
func FlattenContainerAppDapr(input *containerapps.Dapr) []Dapr {
if input == nil {
return []Dapr{}
}
result := Dapr{
AppId: pointer.From(input.AppId),
AppPort: int(pointer.From(input.AppPort)),
}
if appProtocol := input.AppProtocol; appProtocol != nil {
result.AppProtocol = string(*appProtocol)
}
return []Dapr{result}
}

@paolosalvatori
Copy link
Contributor Author

Hi @stephybun, yesterday I successfully tested the change locally without the need to make any change in the ExpandContainerAppDapr or the FlattenContainerAppDapr functions with two container apps, both using Dapr, where:

  • the first explicitly set a value for app_port property in the Dapr block
  • the second one did not specify any value for the app_port property in the Dapr block

The app_port is an integer, optional property, just like the failure_count_threshold that does not require any special code in the expandContainerAppStartupProbe and flattenContainerAppStartupProbe functions.

In my last commit, I added a new test function called completeWithNoAppPort to the container_app_resource_test.go file. Please check and let me know if I need to make other changes.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @paolosalvatori - LGTM 🦕

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

THanks @paolosalvatori ! LGTM 🥡

@paolosalvatori

This comment was marked as off-topic.

@jackofallops jackofallops self-assigned this Feb 27, 2023
@paolosalvatori

This comment was marked as off-topic.

@paolosalvatori
Copy link
Contributor Author

Hi @jackofallops / @katbyte

I added the following functions. Please let me know if this is what you have in mind. If not, please suggest a code change. I'm deploying the complete configuration as a baseline, then I remove the dapr.app_port and finally check that its value is empty.

func TestAccContainerAppResource_removeDaprAppPort(t *testing.T) {
	data := acceptance.BuildTestData(t, "azurerm_container_app", "test")
	r := ContainerAppResource{}

	data.ResourceTest(t, r, []acceptance.TestStep{
		{
			Config: r.complete(data, "rev1"),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
			),
		},
		data.ImportStep(),
		{
			Config: r.completeUpdate_withNoDaprAppPort(data, "rev2"),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).Key("dapr.app_port").IsEmpty(),
			),
		},
		data.ImportStep(),
	})
}

func (r ContainerAppResource) completeUpdate_withNoDaprAppPort(data acceptance.TestData, revisionSuffix string) string {
	return fmt.Sprintf(`
%s

resource "azurerm_container_app" "test" {
  name                         = "acctest-capp-%[2]d"
  resource_group_name          = azurerm_resource_group.test.name
  container_app_environment_id = azurerm_container_app_environment.test.id
  revision_mode                = "Multiple"

  template {
    container {
      name  = "acctest-cont-%[2]d"
      image = "jackofallops/azure-containerapps-python-acctest:v0.0.1"

      cpu    = 0.5
      memory = "1Gi"

      readiness_probe {
        transport               = "HTTP"
        port                    = 5000
        path                    = "/uptime"
        timeout                 = 2
        failure_count_threshold = 1
        success_count_threshold = 1

        header {
          name  = "Cache-Control"
          value = "no-cache"
        }
      }

      liveness_probe {
        transport = "HTTP"
        port      = 5000
        path      = "/health"

        header {
          name  = "Cache-Control"
          value = "no-cache"
        }

        initial_delay           = 5
        timeout                 = 2
        failure_count_threshold = 3
      }

      startup_probe {
        transport               = "TCP"
        port                    = 5000
        timeout                 = 5
        failure_count_threshold = 1
      }

      volume_mounts {
        name = azurerm_container_app_environment_storage.test.name
        path = "/tmp/testdata"
      }
    }

    volume {
      name         = azurerm_container_app_environment_storage.test.name
      storage_type = "AzureFile"
      storage_name = azurerm_container_app_environment_storage.test.name
    }

    min_replicas = 1
    max_replicas = 4

    revision_suffix = "%[3]s"
  }

  ingress {
    allow_insecure_connections = true
    external_enabled           = true
    target_port                = 5000
    transport                  = "auto"

    traffic_weight {
      latest_revision = true
      percentage      = 20
    }

    traffic_weight {
      revision_suffix = "rev1"
      percentage      = 80
    }
  }

  registry {
    server               = azurerm_container_registry.test.login_server
    username             = azurerm_container_registry.test.admin_username
    password_secret_name = "registry-password"
  }

  secret {
    name  = "registry-password"
    value = azurerm_container_registry.test.admin_password
  }

  secret {
    name  = "rick"
    value = "morty"
  }

  dapr {
    app_id       = "acctest-cont-%[2]d"
  }

  tags = {
    foo     = "Bar"
    accTest = "1"
  }
}
`, r.templatePlusExtras(data), data.RandomInteger, revisionSuffix)
}

@paolosalvatori
Copy link
Contributor Author

Hi @jackofallops / @katbyte can you please review my change and let me know if it works? I couldn't test the latest test function.

@paolosalvatori

This comment was marked as off-topic.

@paolosalvatori
Copy link
Contributor Author

@stephybun I committed your suggestion, thanks!

@katbyte
Copy link
Collaborator

katbyte commented Mar 20, 2023

@paolosalvatori - looks like the test check is failing with

------- Stdout: -------
=== RUN   TestAccContainerAppResource_removeDaprAppPort
=== PAUSE TestAccContainerAppResource_removeDaprAppPort
=== CONT  TestAccContainerAppResource_removeDaprAppPort
    testcase.go:110: Step 3/4 error: Check failed: Check 1/1 error: azurerm_container_app.test: Attribute 'dapr.0.app_port' expected "", got "0"
--- FAIL: TestAccContainerAppResource_removeDaprAppPort (1017.08s)
FAIL

should just need to update the test check?

@paolosalvatori
Copy link
Contributor Author

@katbyte @jackofallops can you please check the PR now? It looks to me that now all the tests succeed

@jackofallops
Copy link
Member

@katbyte @jackofallops can you please check the PR now? It looks to me that now all the tests succeed

Hi @paolosalvatori - I think the most recent change is incorrect, it refers to an invalid path so is not testing the correct item, and should be:

	check.That(data.ResourceName).Key("dapr.0.app_port").HasValue("0")

as an empty value for that property in state would be '0'.

If you can update that, I'll run this through our CI again and I think we'll be good to go.

Thanks!

@paolosalvatori
Copy link
Contributor Author

paolosalvatori commented Mar 28, 2023

@katbyte @jackofallops can you please check the PR now? It looks to me that now all the tests succeed

Hi @paolosalvatori - I think the most recent change is incorrect, it refers to an invalid path so is not testing the correct item, and should be:

	check.That(data.ResourceName).Key("dapr.0.app_port").HasValue("0")

as an empty value for that property in state would be '0'.

If you can update that, I'll run this through our CI again and I think we'll be good to go.

Thanks!

Hi @jackofallops please check now

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

thanks @paolosalvatori ! tests pass now LGTM 🚟

@katbyte katbyte merged commit c8c7695 into hashicorp:main Apr 1, 2023
@github-actions github-actions bot added this to the v3.51.0 milestone Apr 1, 2023
katbyte added a commit that referenced this pull request Apr 1, 2023
katbyte added a commit that referenced this pull request Apr 1, 2023
@paolosalvatori
Copy link
Contributor Author

Thanks @katbyte / @jackofallops for the collaboration.

@github-actions
Copy link

This functionality has been released in v3.51.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants