Adds outbound_ip_addresses attribute to app_service #700
Conversation
There was a problem hiding this comment.
Hey @cloudify
Thanks for this PR - I've taken a look and left a couple of comments in-line - if we can add a field to the Schema for this then this should be good to merge :)
Thanks!
| @@ -168,6 +168,8 @@ The following attributes are exported: | |||
|
|
|||
| * `default_site_hostname` - The Default Hostname associated with the App Service - such as `mysite.azurewebsites.net` | |||
|
|
|||
| * `outbound_ip_addresses` - A comma separated list of outbound IP addresses - such as `52.23.25.3,52.143.43.12.5` | |||
There was a problem hiding this comment.
minor I think we want to remove the last segment from the second IPv4 address :)
| @@ -388,6 +388,7 @@ func resourceArmAppServiceRead(d *schema.ResourceData, meta interface{}) error { | |||
| d.Set("client_affinity_enabled", props.ClientAffinityEnabled) | |||
| d.Set("enabled", props.Enabled) | |||
| d.Set("default_site_hostname", props.DefaultHostName) | |||
| d.Set("outbound_ip_addresses", props.OutboundIPAddresses) | |||
There was a problem hiding this comment.
can we add a matching Schema field for this (otherwise this won't actually set anything) - there's an example of this here, and it'll need to be Computed like the default_site_hostname field since it's only set once the app is created
cloudify
force-pushed
the
appservice_outboundips
branch
from
44e81d9
to
b1869a2
Compare
|
Thanks @tombuildsstuff , I've updated the PR with your suggestions. |
There was a problem hiding this comment.
hey @cloudify
Thanks for pushing those updates - this now LGTM. Taking another look at this I'm wondering if it'd be useful to make the outbound_ip_addresses field a list rather than a string containing comma-separated values, so that users don't need to handle this themselves (e.g. so that we split the value into an array) - what do you think?
Thanks!
|
Hi @tombuildsstuff , I thought about that too, but I opted to keep the logic in the provider as simple as possible and if a list is needed, use the split function provided by Terraform. |
|
@cloudify sounds sensible - we can always change this in a future release if needed. I'll kick off the test suite now - but this LGTM, thanks for this contribution :) |
|
Tests pass:
|
|
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! |
ghost
locked and limited conversation to collaborators
No description provided.