-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New Data Source: azurerm_virtual_network
#533
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.
Hey @dcaro
Thanks for opening this PR - I've taken a look through and left some comments in-line but this is off to a good start; the only thing missing that I can see at this point is some documentation - would it be possible to add some for this Data Source?
Thanks!
azurerm/data_source_vnet.go
Outdated
|
||
resp, err := vnetClient.Get(resGroup, name, "") | ||
if err != nil { | ||
if resp.StatusCode == http.StatusNotFound { |
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.
can we change this to use utils.ResponseWasNotFound()
to handle the connection being dropped (where this will currently crash)?
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.
Make sense, will do so.
azurerm/data_source_vnet.go
Outdated
if resp.StatusCode == http.StatusNotFound { | ||
d.SetId("") | ||
} | ||
return fmt.Errorf("Error making Read request on Azure virtual network %s: %s", name, err) |
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.
can we change this second formatting argument to %+v
- which outputs the full exception, rather than just the friendly message (sometimes the actual error is hidden)
azurerm/data_source_vnet.go
Outdated
func flattenVnetAddressPrefixes(input *[]string) []interface{} { | ||
prefixes := make([]interface{}, 0) | ||
|
||
for _, prefix := range *input { |
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.
in order to handle a potential crash - can we update this to:
if prefixes := input; prefixes != nil {
for _, prefix := range *prefixes {
# ..
}
}
at the moment - if input
isn't returned (such as, for a resource created through an older API version) - this will crash (this check should fix that)
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.
@tombuildsstuff let me know if the change that I made here is what you are expecting.
azurerm/data_source_vnet.go
Outdated
func flattenVnetSubnetsNames(input *[]network.Subnet) []interface{} { | ||
subnets := make([]interface{}, 0) | ||
|
||
for _, subnet := range *input { |
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.
(same here)
azurerm/data_source_vnet.go
Outdated
func flattenVnetPeerings(input *[]network.VirtualNetworkPeering) map[string]interface{} { | ||
output := make(map[string]interface{}, 0) | ||
|
||
for _, vnetpeering := range *input { |
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.
(same here)
azurerm/provider.go
Outdated
@@ -80,6 +80,7 @@ func Provider() terraform.ResourceProvider { | |||
"azurerm_snapshot": dataSourceArmSnapshot(), | |||
"azurerm_subnet": dataSourceArmSubnet(), | |||
"azurerm_subscription": dataSourceArmSubscription(), | |||
"azurerm_vnet": dataSourceArmVnet(), |
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.
can we name this azurerm_virtual_network
to match the other resource?
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.
of course, done :)
azurerm/data_source_vnet.go
Outdated
//"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" | ||
) | ||
|
||
func dataSourceArmVnet() *schema.Resource { |
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.
can we rename this to dataSourceArmVirtualNetwork
to match the other resource?
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.
Including suggested changes from @tombuildsstuff and added documentation.
azurerm/data_source_vnet.go
Outdated
|
||
resp, err := vnetClient.Get(resGroup, name, "") | ||
if err != nil { | ||
if resp.StatusCode == http.StatusNotFound { |
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.
Make sense, will do so.
azurerm/provider.go
Outdated
@@ -80,6 +80,7 @@ func Provider() terraform.ResourceProvider { | |||
"azurerm_snapshot": dataSourceArmSnapshot(), | |||
"azurerm_subnet": dataSourceArmSubnet(), | |||
"azurerm_subscription": dataSourceArmSubscription(), | |||
"azurerm_vnet": dataSourceArmVnet(), |
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.
of course, done :)
azurerm/data_source_vnet.go
Outdated
func flattenVnetAddressPrefixes(input *[]string) []interface{} { | ||
prefixes := make([]interface{}, 0) | ||
|
||
for _, prefix := range *input { |
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.
@tombuildsstuff let me know if the change that I made here is what you are expecting.
dns_servers = ["10.0.0.4"] | ||
|
||
subnet { | ||
name = "subnet1" |
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.
Use spaces to keep indentation
} | ||
|
||
resource "azurerm_virtual_network_peering" "test1" { | ||
name = "peer-1to2" |
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.
Use spaces to keep indentation (same for next 3 lines)
There are some indentation that could be fixed but this LGTM and I need this feature. |
@pmarques I have updated the code to include spaces instead of tabs. @tombuildsstuff let me know if you see any issues. |
azurerm_virtual_network
@@ -0,0 +1,37 @@ | |||
--- |
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.
just noticed there isn't a sidebar entry for this Data Source - I've pushed a commit to fix this :)
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.
Hey @dcaro
Thanks for pushing the latest changes - I've taken another look through and have left some comments inline, but this mostly LGTM. I've also pushed a commit to add the Data Source to the sidebar - however upon running the tests I noticed the Peering test failed:
$ acctests azurerm TestAccDataSourceArmVirtualNetwork_
=== RUN TestAccDataSourceArmVirtualNetwork_basic
--- PASS: TestAccDataSourceArmVirtualNetwork_basic (77.07s)
=== RUN TestAccDataSourceArmVirtualNetwork_peering
--- FAIL: TestAccDataSourceArmVirtualNetwork_peering (95.24s)
testing.go:459: Step 0 error: Check failed: Check 3/3 error: data.azurerm_virtual_network.test: Attribute 'vnet_peerings.%' expected "1", got "0"
FAIL
exit status 1
FAIL github.com/terraform-providers/terraform-provider-azurerm/azurerm 172.327s
If we can fix up the broken test this PR otherwise looks good to merge :)
Thanks!
ping @dcaro - have you had a chance to look into this yet? :) |
@tombuildsstuff I have some issues with the second test. I traced the response from the REST API and I see that the peering is correctly provisioned when running the testAccDataSourceArmVirtualNetwork_peering function. The interpretation of the response is the issue. I'm trying to narrow down the function where this fails but so far, to me, it looks like an issue in terraform. Any help on this would be welcome. |
@dcaro sure - used the configuration from the failing test (and added some variables) to take a look into this:
In order to understand what's going on, we can inspect what the Terraform Graph for this Terraform Configuration using the command Given the We can use the
The Terraform Graph for this looks like so: which means that the I've confirmed that Thanks! |
Thanks so much @tombuildsstuff I will update my PR today ! |
@tombuildsstuff when I introduce the dependency, I have the following error when I run the test:
[properties of the resource group]
[properties of the virtual network]
[properties of the virtual network]
[properties of the virtual network peering]
|
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 this 👍
@dcaro so after digging into this, it appears that a data source with a
I've pushed a commit to add support for this and this Data Source now LGTM - thanks for this :) Tests pass:
Thanks! |
@tombuildsstuff do you have any details about the next release date? I would like to have this available without compiling my own. |
@pmarques "soon" - it'll be next week, but I'd rather not commit to a specific day/date - hope that helps :) |
* Provision sample for ASP.NET on azure_rm_app_service * Added vnet datasource * add identity property to vm * refactor, tests and docs * added vnet_peering * changing to TypeMap * Updating the Provider block * Variable consistency and removing unused variables * Changed to azure_virtual_network, added crash control and added documentation. * vmss: Support for updating the customData field Fixes hashicorp#61 Fixes hashicorp#490 * Updating to include hashicorp#559 * Support for Auto Inflating ``` $ acctests azurerm TestAccAzureRMEventHubNamespace_maximumThroughputUnits === RUN TestAccAzureRMEventHubNamespace_maximumThroughputUnits --- PASS: TestAccAzureRMEventHubNamespace_maximumThroughputUnits (202.41s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 202.432s ``` * New Resource: `azurerm_network_watcher` ``` $ acctests azurerm TestAccAzureRMNetworkWatcher_ === RUN TestAccAzureRMNetworkWatcher_importBasic --- PASS: TestAccAzureRMNetworkWatcher_importBasic (75.79s) === RUN TestAccAzureRMNetworkWatcher_importComplete --- PASS: TestAccAzureRMNetworkWatcher_importComplete (69.85s) === RUN TestAccAzureRMNetworkWatcher_basic --- PASS: TestAccAzureRMNetworkWatcher_basic (69.62s) === RUN TestAccAzureRMNetworkWatcher_complete --- PASS: TestAccAzureRMNetworkWatcher_complete (72.16s) === RUN TestAccAzureRMNetworkWatcher_update --- PASS: TestAccAzureRMNetworkWatcher_update (81.75s) === RUN TestAccAzureRMNetworkWatcher_disappears --- PASS: TestAccAzureRMNetworkWatcher_disappears (94.38s) PASS ok ``` * Updating to include hashicorp#569 * Hotfix: upgrade packages under go-autorest to be v9.4.1. Intergrate with latest version of go-autorest to read access tokens through new way customized through environment variable. The old behavior on local shell will be kept. Notice: for Azure Cloud Shell user, please make sure that they're using latest patched provider. * Vendoring the Locks SDK * New Resource: `azurerm_management_lock` Note: As the Subscription specific Locks will break other tests; these tests need to be run individually. As such I've introduced the `TF_ACC_SUBSCRIPTION_PARALLEL_LOCK` environment variable for this purpose. Tests pass: ``` $ TF_ACC_SUBSCRIPTION_PARALLEL_LOCK=1 acctests azurerm TestAccAzureRMManagementLock_ === RUN TestAccAzureRMManagementLock_importResourceGroupReadOnlyBasic --- PASS: TestAccAzureRMManagementLock_importResourceGroupReadOnlyBasic (61.52s) === RUN TestAccAzureRMManagementLock_importResourceGroupReadOnlyComplete --- PASS: TestAccAzureRMManagementLock_importResourceGroupReadOnlyComplete (58.75s) === RUN TestAccAzureRMManagementLock_importResourceGroupCanNotDeleteBasic --- PASS: TestAccAzureRMManagementLock_importResourceGroupCanNotDeleteBasic (53.38s) === RUN TestAccAzureRMManagementLock_importResourceGroupCanNotDeleteComplete --- PASS: TestAccAzureRMManagementLock_importResourceGroupCanNotDeleteComplete (46.87s) === RUN TestAccAzureRMManagementLock_importPublicIPCanNotDeleteBasic --- PASS: TestAccAzureRMManagementLock_importPublicIPCanNotDeleteBasic (80.46s) === RUN TestAccAzureRMManagementLock_importPublicIPReadOnlyBasic --- PASS: TestAccAzureRMManagementLock_importPublicIPReadOnlyBasic (68.53s) === RUN TestAccAzureRMManagementLock_resourceGroupReadOnlyBasic --- PASS: TestAccAzureRMManagementLock_resourceGroupReadOnlyBasic (61.24s) === RUN TestAccAzureRMManagementLock_resourceGroupReadOnlyComplete --- PASS: TestAccAzureRMManagementLock_resourceGroupReadOnlyComplete (64.10s) === RUN TestAccAzureRMManagementLock_resourceGroupCanNotDeleteBasic --- PASS: TestAccAzureRMManagementLock_resourceGroupCanNotDeleteBasic (72.49s) === RUN TestAccAzureRMManagementLock_resourceGroupCanNotDeleteComplete --- PASS: TestAccAzureRMManagementLock_resourceGroupCanNotDeleteComplete (113.71s) === RUN TestAccAzureRMManagementLock_publicIPReadOnlyBasic --- PASS: TestAccAzureRMManagementLock_publicIPReadOnlyBasic (64.05s) === RUN TestAccAzureRMManagementLock_publicIPCanNotDeleteBasic --- PASS: TestAccAzureRMManagementLock_publicIPCanNotDeleteBasic (94.53s) === RUN TestAccAzureRMManagementLock_subscriptionReadOnlyBasic --- PASS: TestAccAzureRMManagementLock_subscriptionReadOnlyBasic (17.98s) === RUN TestAccAzureRMManagementLock_subscriptionCanNotDeleteBasic --- PASS: TestAccAzureRMManagementLock_subscriptionCanNotDeleteBasic (15.20s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 872.839s ``` Fixes hashicorp#23 * Updating to include hashicorp#573 * Updating to include hashicorp#571 * Adding validation for the locks name Tests: ``` $ acctests azurerm TestValidateManagementLockName === RUN TestValidateManagementLockName --- PASS: TestValidateManagementLockName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.020s ``` * Linting * Updating to include hashicorp#575 * Updating the changelog for consistency * removed tabs, used spaces * add test for issue hashicorp#450 * Updated the way user agent string gets assigned. * Changed code to make it more readable. * pr tweaks * Avoid out of index errors when flattening image data disks. * Updating to include hashicorp#587 * Updating to include hashicorp#589 * Conditional loading of the Subscription ID / Tenant ID / Environment * Refactoring the provider block to support determining the TenantID/Environment from the SubscriptionID Splitting out the authentication logic into a helpers folder Also adding unit tests for these - which pass: ``` $ go test . -v === RUN TestAzureFindValidAccessTokenForTenant_InvalidDate --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidDate (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_Expired 2017/11/30 15:02:01 [DEBUG] Token "7cabcf30-8dca-43f9-91e6-fd56dfb8632f" has expired --- PASS: TestAzureFindValidAccessTokenForTenant_Expired (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ExpiringIn --- PASS: TestAzureFindValidAccessTokenForTenant_ExpiringIn (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain 2017/11/30 15:02:01 [DEBUG] Resource "https://portal.azure.com/" isn't a management domain --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_DifferentTenant 2017/11/30 15:02:01 [DEBUG] Resource "https://management.core.windows.net/" isn't for the correct Tenant --- PASS: TestAzureFindValidAccessTokenForTenant_DifferentTenant (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_NoTokens --- PASS: TestAzureFindValidAccessTokenForTenant_NoTokens (0.00s) === RUN TestAzureCLIProfileFindDefaultSubscription --- PASS: TestAzureCLIProfileFindDefaultSubscription (0.00s) === RUN TestAzureCLIProfileFindSubscription --- PASS: TestAzureCLIProfileFindSubscription (0.00s) === RUN TestAzurePopulateSubscriptionFromCLIProfile_Missing --- PASS: TestAzurePopulateSubscriptionFromCLIProfile_Missing (0.00s) === RUN TestAzurePopulateSubscriptionFromCLIProfile_NoDefault --- PASS: TestAzurePopulateSubscriptionFromCLIProfile_NoDefault (0.00s) === RUN TestAzurePopulateSubscriptionFromCLIProfile_Default --- PASS: TestAzurePopulateSubscriptionFromCLIProfile_Default (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_Empty --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_Empty (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_MissingSubscription --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_MissingSubscription (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_PopulateEnvironment --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_PopulateEnvironment (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_NormaliseAndPopulateEnvironment --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_NormaliseAndPopulateEnvironment (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_PopulateTenantId --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_PopulateTenantId (0.00s) === RUN TestAzurePopulateTenantAndEnvironmentFromCLIProfile_Complete --- PASS: TestAzurePopulateTenantAndEnvironmentFromCLIProfile_Complete (0.00s) === RUN TestAzurePopulateFromAccessToken_Missing --- PASS: TestAzurePopulateFromAccessToken_Missing (0.00s) === RUN TestAzurePopulateFromAccessToken_Exists --- PASS: TestAzurePopulateFromAccessToken_Exists (0.00s) === RUN TestAzureEnvironmentNames --- PASS: TestAzureEnvironmentNames (0.00s) === RUN TestAzureValidateBearerAuth --- PASS: TestAzureValidateBearerAuth (0.00s) === RUN TestAzureValidateServicePrincipal --- PASS: TestAzureValidateServicePrincipal (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/authentication 0.012s ``` * Fixing the build * Remove the field marked as "Removed" according to hashicorp#572. * Upgrading to v11.2.2-beta of the Azure SDK for Go * Updating to include hashicorp#593 * Fixing the Management Lock validation * Adding a default value for the identity field * Updating to include hashicorp#482 * Updating to include hashicorp#574 * Adding settings to the hash Test passes: ``` $ acctests azurerm TestAccAzureRMVirtualMachineScaleSet_extensionUpdate === RUN TestAccAzureRMVirtualMachineScaleSet_extensionUpdate --- PASS: TestAccAzureRMVirtualMachineScaleSet_extensionUpdate (593.13s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 593.153s ``` * Updating to include hashicorp#609 * Local Network Gateways: support for BGP Settings ``` $ acctests azurerm TestAccAzureRMLocalNetworkGateway_ === RUN TestAccAzureRMLocalNetworkGateway_importBasic --- PASS: TestAccAzureRMLocalNetworkGateway_importBasic (82.23s) === RUN TestAccAzureRMLocalNetworkGateway_basic --- PASS: TestAccAzureRMLocalNetworkGateway_basic (81.29s) === RUN TestAccAzureRMLocalNetworkGateway_disappears --- PASS: TestAccAzureRMLocalNetworkGateway_disappears (79.17s) === RUN TestAccAzureRMLocalNetworkGateway_bgpSettings --- PASS: TestAccAzureRMLocalNetworkGateway_bgpSettings (78.70s) === RUN TestAccAzureRMLocalNetworkGateway_bgpSettingsDisable --- PASS: TestAccAzureRMLocalNetworkGateway_bgpSettingsDisable (96.18s) === RUN TestAccAzureRMLocalNetworkGateway_bgpSettingsEnable --- PASS: TestAccAzureRMLocalNetworkGateway_bgpSettingsEnable (97.39s) === RUN TestAccAzureRMLocalNetworkGateway_bgpSettingsComplete --- PASS: TestAccAzureRMLocalNetworkGateway_bgpSettingsComplete (79.68s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 594.680s ``` * Refactoring * Adding an import test for BGP Settings: ``` $ acctests azurerm TestAccAzureRMLocalNetworkGateway_importBGPSettingsComplete === RUN TestAccAzureRMLocalNetworkGateway_importBGPSettingsComplete --- PASS: TestAccAzureRMLocalNetworkGateway_importBGPSettingsComplete (80.96s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 80.987s ``` * Splitting the data source out into it's own step * Minor refactoring * Updating to include hashicorp#533 * Exporting the Default Hostname field * Updating the App Service example to be complete This removes support for Publishing, since the SCM URL's aren't consistent across Sovereign Clouds (China/Germany/Govt etc) Switches to using the new `default_site_hostname` field introduced in hashicorp#612 rather than assuming what it is * Updating to include hashicorp#594 * Updating to include hashicorp#611 * Updating to include hashicorp#612 * Remove leading line break from key_vault_key docs Leading line break causes page metadata to be ignored.
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! |
Adding a data source for vnet in Azure.