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

Normalizes availability_set_id to lowercase to avoid spurious diffs #6768

Merged
merged 1 commit into from
May 19, 2016
Merged

Normalizes availability_set_id to lowercase to avoid spurious diffs #6768

merged 1 commit into from
May 19, 2016

Conversation

Bowbaq
Copy link
Contributor

@Bowbaq Bowbaq commented May 19, 2016

Azure RM seems to return uppercased availability set ids from the API, which causes spurious diffs if the case used in the definition is anything else than uppercase. This normalizes the id to lowercase.

@stack72
Copy link
Contributor

stack72 commented May 19, 2016

Hi @Bowbaq

Thanks for the PR here - it's awesome to see someone else try to fix this. I have been thinking about this a lot and I am not quite sure it is as easy as this. The Azure API is actually case-sensitive i.e.

parsing a resource ID for VirtualMachines will return no result but virtualMachines will (notice the lowercase v here)

Therefore, we need to run some tests on this to make sure that the virtual machine is actually being added to the correct availability_set when using lowercase

Does this make sense? Have you tried the tests?

Paul

@stack72 stack72 added bug provider/azurerm waiting-response An issue/pull request is waiting for a response from the community labels May 19, 2016
@Bowbaq
Copy link
Contributor Author

Bowbaq commented May 19, 2016

Seems to work fine in this case

@stack72
Copy link
Contributor

stack72 commented May 19, 2016

This is good for me :) We can merge it and then let it bake in the nightly tests :) Thanks so much for the work here

@stack72 stack72 removed the waiting-response An issue/pull request is waiting for a response from the community label May 19, 2016
@stack72 stack72 merged commit b33bd42 into hashicorp:master May 19, 2016
@stack72 stack72 mentioned this pull request May 19, 2016
8 tasks
cristicalin pushed a commit to cristicalin/terraform that referenced this pull request May 24, 2016
@dougt1
Copy link

dougt1 commented Aug 15, 2016

Hi @stack72 & @Bowbaq - i think this change may have introduced a bug. I am running v 0.7.0 in Azure (Resource Manager) with 2 VMs in an availability set - and now on every run my location is seen as different (as appears case sensitive) - this results in the forced creation of new resources within the availability group - resulting in the deletion and re-creation of all vm's in the availability set. It doesn't seem to matter what case I specify the location as - on subsequent runs it always returns as a change and wants to recreate the resource group and the VM's within the group.

Results from a terraform plan below:-

-/+ module.availability_set_test.azurerm_availability_set.availability_set
location: "UKSouth2" => "uksouth2" (forces new resource)
name: "availability-set-test" => "availability-set-test"
platform_fault_domain_count: "3" => "3"
platform_update_domain_count: "5" => "5"
resource_group_name: "test-prod-rg" => "test-prod-rg"
tags.%: "1" => "1"
tags.environment: "production" => "production"

Also - when i look at the Travis CI build results, unless I am reading it wrong it looks like there are no tests for Azure RM:-

ok github.com/hashicorp/terraform/builtin/bins/provider-azure 0.006s
? github.com/hashicorp/terraform/builtin/bins/provider-azurerm [no test files]
? github.com/hashicorp/terraform/builtin/bins/provider-chef [no test files]

so maybe we need to look at adding in some automated tests as well?

Thanks.

@stack72
Copy link
Contributor

stack72 commented Aug 15, 2016

Morning @dougt1

So this PR only touched availability_set_id, not location. The issue here is that you are using a non-normalized version of the location. In your case, the lowercase uksouth2 or UK South 2 would work - this specific issue you found is another bug

Also, FYI, the make test is a unit tests thing - we have a full suite of automated tests for AzureRM :) https://travis-ci.org/hashicorp/terraform/builds/152307805

We run these each night

I will try and dig into the location thing - can you open a new issue to mark the work on that?

P.

@ghost
Copy link

ghost commented Apr 23, 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 23, 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.

3 participants