Add possible_outbound_ip_addresses to app service

[tomasaschan]

This is useful for e.g. setting up firewall rules for Azure SQL - since an App Service can be rescheduled, it's possible that the outbound_ip_addresses field output at deploy-time does not catch all IP addresses you need to open up to be future-safe.

Note: I couldn't get the code to build and run tests on my Windows machine (neither from PowerShell, inspecting the makefile to see what was required, nor from WSL), so I've basically just looked at #700 to see what I need to do to support this. (I think this could have been made easier with a .gitattributes and an .editorconfig at the project root, that - at the very least - specify line endings to be what we expect.) Please feel free to request changes if I missed something 😄

[katbyte]

@tomasaschan,

I think this may be better to export as a TypeList rather than a comma separated field. However in that case we should probably also update oubound_ip_addresses to match?

[tomasaschan]

Yeah, I actually thought so too, but decided to follow #700 as closely as possible. The rationale in the discussion there was that it's easy enough to get that list using split if one needs it.

I think it's reasonable that they match, but I don't really care which way as long as they both go the same way :) But since changing outbound_ip_addresses would be a breaking change, while this isn't, I also figured this can be added with less friction, and a new PR that changes the return type of both attributes can be made in the 2.0 scope.

[katbyte]

Rational decision to me @tomasaschan, CSV for now it is!

[metacpp]

Hi @tomasaschan

Thanks for the contribution. In general, it looks good to me, however it appears that you missed adding the schema and documentation for the data source.

And could we add some validations for this new field in data source and resource test ?

[katbyte]

For the test you could just add it to the basic one, like:

func TestAccDataSourceAzureRMAppService_basic(t *testing.T) {
	dataSourceName := "data.azurerm_app_service.test"
	rInt := acctest.RandInt()
	location := testLocation()

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:  func() { testAccPreCheck(t) },
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccDataSourceAppService_basic(rInt, location),
				Check: resource.ComposeTestCheckFunc(
					resource.TestCheckResourceAttrSet(dataSourceName, "app_service_plan_id"),
					resource.TestCheckResourceAttrSet(dataSourceName, "possible_outbound_ip_addresses"),
					resource.TestCheckResourceAttrSet(dataSourceName, "outbound_ip_addresses"),
					resource.TestCheckResourceAttr(dataSourceName, "tags.%", "0"),
				),
			},
		},
	})
}

[tomasaschan]

@katbyte @metacpp Thanks for the review! I honestly hadn't thought about the data source getting the same properties, but of course that makes sense. I've added the new property to the schema for the data source as well.

I've also added a few assertions to the basic tests for both resource and data source, as well as documentation of these properties to the data source (it seems the reason I missed it in the first place is that the outbound_ip_addresses property was not documented either). I hope the tests look good like this, as I haven't been able to run them locally.

[tombuildsstuff]

hey @tomasaschan

Thanks for pushing those changes - this now LGTM, I'll kick off the tests shortly 👍

Thanks!

dismissing since changes have been pushed

[tombuildsstuff]

Data Source tests pass:

[tombuildsstuff]

Resource tests pass:

[tomasaschan]

Woop woop! Thanks for taking this - will significantly simplify my deployments, once released 🎉

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!