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

provider/azurerm: set resource_group_name on resource import #9073

Merged
merged 15 commits into from
Oct 7, 2016

Conversation

carinadigital
Copy link
Contributor

Fix for GH-9072

I will add more resources as I test them.

@stack72
Copy link
Contributor

stack72 commented Sep 27, 2016

Hi @carinadigital

Thanks for the work here, this is a great addition!!! - if you are getting these added - can you also remove the ImportStateVerifyIgnore: []string{"resource_group_name"}, from the import_arm_*_test.go files? :)

Thanks

Paul

@carinadigital carinadigital changed the title [WIP] provider/azurerm set resource_group_name on resource import [WIP] provider/azurerm: set resource_group_name on resource import Sep 27, 2016
@carinadigital
Copy link
Contributor Author

carinadigital commented Sep 27, 2016

There's another issue thrown up by the tests. The capitalisation of resource_group_name from the storage account acceptance fails due to the capitalisation.

The resource_group_name of azurerm_storage_account is always stored in lowercase.
However, the name attribute of azurerm_resource_group contains capitalisation.
Therefore where azurerm_resource_group.[name].name is used it will see a capitalisation diff.

@carinadigital
Copy link
Contributor Author

@stack72 Would adding a DiffSuppressFunc() https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go#L62 to resource_group_name inside most azurerm resources schema be a good solution to this?

It should just ignore the diff when the .ToLower() is equal. This will allow the name to stay capitalised for display purposes.

@carinadigital
Copy link
Contributor Author

I've gone through the first half of the importable resource today. I'll try to complete them tomorrow.

@stack72
Copy link
Contributor

stack72 commented Sep 27, 2016

@carinadigital this is awesome :)

@carinadigital
Copy link
Contributor Author

carinadigital commented Sep 28, 2016

I've found 4 resources that always store resource_group_name in lowercase. I'm not sure why this is the case.

azurerm_storage_account
azurerm_dns_zone
azurerm_traffic_manager_profile
azurerm_traffic_manager_endpoint

I've not yet completed a full run of the acceptance tests to see if there are anymore.

@carinadigital carinadigital changed the title [WIP] provider/azurerm: set resource_group_name on resource import provider/azurerm: set resource_group_name on resource import Sep 28, 2016
@carinadigital
Copy link
Contributor Author

@stack72 This is ready for a review.
This should have an azurerm acceptance test run. FYI it took over 2hours on my machine.

Andreas Kyrris added 15 commits October 3, 2016 15:46
Some azurerm resources store resource_group_name in lowercase only.
Other store resource_group_name using lower and upper case.
Ensure that all test cases use capitalisation in resource_group_name
to find errors in diffs due to capitalisation.

Some resource_group_name were refactored to match naming scheme
across the azurerm tests.
Some resource fail due to resource_group_name always being stored as
lowercase. For those resources we add the case insensitive diff
function.
@stack72
Copy link
Contributor

stack72 commented Oct 7, 2016

Hi @carinadigital

Thanks for the work here - this LGTM! All tests are passing as expected:

% make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=_import'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/07 12:48:38 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=_import -timeout 120m
=== RUN   TestAccAzureRMAvailabilitySet_importBasic
--- PASS: TestAccAzureRMAvailabilitySet_importBasic (100.66s)
=== RUN   TestAccAzureRMDnsZone_importBasic
--- PASS: TestAccAzureRMDnsZone_importBasic (92.71s)
=== RUN   TestAccAzureRMLocalNetworkGateway_importBasic
--- PASS: TestAccAzureRMLocalNetworkGateway_importBasic (124.95s)
=== RUN   TestAccAzureRMNetworkSecurityGroup_importBasic
--- PASS: TestAccAzureRMNetworkSecurityGroup_importBasic (126.42s)
=== RUN   TestAccAzureRMNetworkSecurityRule_importBasic
--- PASS: TestAccAzureRMNetworkSecurityRule_importBasic (167.86s)
=== RUN   TestAccAzureRMPublicIpStatic_importBasic
--- PASS: TestAccAzureRMPublicIpStatic_importBasic (127.65s)
=== RUN   TestAccAzureRMResourceGroup_importBasic
--- PASS: TestAccAzureRMResourceGroup_importBasic (76.44s)
=== RUN   TestAccAzureRMServiceBusSubscription_importBasic
--- PASS: TestAccAzureRMServiceBusSubscription_importBasic (365.62s)
=== RUN   TestAccAzureRMServiceBusTopic_importBasic
--- PASS: TestAccAzureRMServiceBusTopic_importBasic (366.95s)
=== RUN   TestAccAzureRMSqlFirewallRule_importBasic
--- PASS: TestAccAzureRMSqlFirewallRule_importBasic (127.38s)
=== RUN   TestAccAzureRMStorageAccount_importBasic
--- PASS: TestAccAzureRMStorageAccount_importBasic (139.00s)
=== RUN   TestAccAzureRMTrafficManagerEndpoint_importBasic
--- PASS: TestAccAzureRMTrafficManagerEndpoint_importBasic (144.72s)
=== RUN   TestAccAzureRMTrafficManagerProfile_importBasic
--- PASS: TestAccAzureRMTrafficManagerProfile_importBasic (82.50s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_importBasic
--- PASS: TestAccAzureRMVirtualNetworkPeering_importBasic (213.47s)
=== RUN   TestAccAzureRMVirtualNetwork_importBasic
--- PASS: TestAccAzureRMVirtualNetwork_importBasic (122.85s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    2379.201s

Paul

@stack72 stack72 merged commit 250be11 into hashicorp:master Oct 7, 2016
@carinadigital carinadigital deleted the GH-9072 branch October 21, 2016 13:11
@ghost
Copy link

ghost commented Apr 21, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants