`azurerm_cdn_endpoint` - Standardize the resource ID to be case sensitive #8237

magodo · 2020-08-25T02:03:22Z

This is a workaround for #Azure/azure-rest-api-specs#10576.

Test Result

💢 make testacc TEST=./azurerm/internal/services/cdn/tests TESTARGS="-run='TestAccAzureRMCdnEndpoint_dnsAlias'"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...                                                                          
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/cdn/tests -v -run='TestAccAzureRMCdnEndpoint_dnsAlias' -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMCdnEndpoint_dnsAlias                                                                           
=== PAUSE TestAccAzureRMCdnEndpoint_dnsAlias            
=== CONT  TestAccAzureRMCdnEndpoint_dnsAlias               
--- PASS: TestAccAzureRMCdnEndpoint_dnsAlias (477.26s)
PASS                                                                                                                   
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/cdn/tests   477.286s

…tive This is a workaround for #Azure/azure-rest-api-specs#10576. Fixes: hashicorp#8191

tombuildsstuff

hey @magodo

Thanks for this PR - if we can add a couple of state migrations to patch the ID here then this otherwise LGTM 👍

Thanks!

azurerm/internal/services/cdn/cdn_endpoint_resource.go

azurerm/internal/services/cdn/cdn_profile_resource.go

…sources

magodo · 2020-08-25T06:18:34Z

@tombuildsstuff I have added the migration logic, thank you for the example link!

tombuildsstuff

LGTM 👍

tombuildsstuff

Tests picked up a circular reference

azurerm/internal/services/cdn/cdn_endpoint_resource.go

azurerm/internal/services/cdn/cdn_profile_data_source.go

azurerm/internal/services/cdn/cdn_profile_resource.go

magodo · 2020-08-25T09:24:21Z

@tombuildsstuff I have resolved the CI errors, PTAL!

tombuildsstuff

Unfortunately due to the design of the new state migration functions we have to entirely duplicate the schema as a point-in-time copy - so we're unable to reuse this in the resource - incase this differs in the future

If we can duplicate that then this should otherwise be fine though 👍

tombuildsstuff · 2020-08-25T11:23:08Z

azurerm/internal/services/cdn/cdn_profile_resource.go

+			return err
+		}),
+
+		Schema: tmpResource.Schema,


the schema needs to remain here - and be entirely duplicated for the migration; else if these differ in the future the original state migration schema is no longer valid - so unfortunately these cannot be reused and must be copy/pasted as a point-in-time reference to the current schema

As this is a not a schema migration essentially, it is just the ID got migrated. I think it is fine to reuse the schema at this point.

If the schema got changed in the future, it should not impact the fact that the old ID format will be transitioned to the new one as long as the schema_version in state is 0. On the other hand, if the schema introduced a breaking change and we want to keep backward compatibility, then we can do a point-in-time copy at that time (like a copy-on-write style).

In this way, it is like that the version 0 schema keeps evolving until we decide to snapshot it in a fixed version at the time that we really want to do some schema level migration.

What do you think?

The state migration system assumes there's a fixed point-in-time schema (e.g. only these fields exist, so unfortunately any additions would be a breaking change). Whilst we could technically reuse this at this point - the amount of change in this codebase makes it plausable we could unintentionally introduce a breaking change later - so it's better to follow the convention used by other state migrations and duplicate this.

Whilst it's unfortunate the state migration system is designed to require a schema (and we've raised this with the Plugin SDK Team, since this shouldn't be necessary, or should only require the specific attributes being modified for deserialization/serialization) - duplicating this removes the possibility of these unintentional bugs here, which are going to be incredibly hard to capture.

As such we're better to duplicate this schema in both instances.

tombuildsstuff · 2020-08-25T11:23:16Z

azurerm/internal/services/cdn/cdn_endpoint_resource.go

+			return err
+		}),
+
+		Schema: tmpResource.Schema,


the schema needs to remain here - and be entirely duplicated for the migration; else if these differ in the future the original state migration schema is no longer valid - so unfortunately these cannot be reused and must be copy/pasted as a point-in-time reference to the current schema

magodo · 2020-08-26T13:33:08Z

@tombuildsstuff I have added the dedicated PIT V0 schema as you suggested, please take another look! Thanks!

tombuildsstuff

hey @magodo

Thanks for pushing those changes - I've taken a look through and left some more comments inline but if we can duplicate the remaining schema here (and remove the validation funcs since these are unnecessary) then this should otherwise be good 👍🏻

Thanks!

azurerm/internal/services/cdn/migration/cdn_endpoint.go

azurerm/internal/services/cdn/cdn_profile_resource.go

…n in V0 schema

magodo · 2020-08-27T09:13:13Z

@tombuildsstuff Done, I also expand the function invocation for fields like resource_group_name, location and tags.

tombuildsstuff

If we can make these consistent (these were missed in the last review cycle) then this otherwise LGTM 👍

azurerm/internal/services/cdn/parse/cdn_profile.go

azurerm/internal/services/cdn/parse/cdn_endpoint.go

magodo · 2020-08-31T01:47:36Z

@tombuildsstuff I have renamed the parameters, please take another look!

tombuildsstuff

LGTM 👍

tombuildsstuff · 2020-09-02T08:48:55Z

Tests pass:

ghost · 2020-09-04T03:30:41Z

This has been released in version 2.26.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.26.0"
}
# ... other configuration ...

ghost · 2020-10-02T14:50:18Z

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!

azurerm_cnd_endpoint - Standardize the resource ID to be case sensi…

e5701a2

…tive This is a workaround for #Azure/azure-rest-api-specs#10576. Fixes: hashicorp#8191

ghost added the size/M label Aug 25, 2020

magodo mentioned this pull request Aug 25, 2020

azurerm_dns_x_record: ignore case on target_resource_id #8218

Closed

conform to the Formatter style

8aa6298

tombuildsstuff reviewed Aug 25, 2020

View reviewed changes

azurerm/internal/services/cdn/cdn_endpoint_resource.go Show resolved Hide resolved

azurerm/internal/services/cdn/cdn_profile_resource.go Show resolved Hide resolved

tombuildsstuff added bug service/cdn waiting-response labels Aug 25, 2020

add migration logic to cdn profile/endpoint, both managed and data re…

ab8a159

…sources

ghost added size/XL and removed size/M labels Aug 25, 2020

tombuildsstuff approved these changes Aug 25, 2020

View reviewed changes

tombuildsstuff removed the waiting-response label Aug 25, 2020

tombuildsstuff suggested changes Aug 25, 2020

View reviewed changes

azurerm/internal/services/cdn/cdn_endpoint_resource.go Show resolved Hide resolved

azurerm/internal/services/cdn/cdn_profile_data_source.go Outdated Show resolved Hide resolved

azurerm/internal/services/cdn/cdn_profile_resource.go Show resolved Hide resolved

tombuildsstuff assigned magodo Aug 25, 2020

magodo added 2 commits August 25, 2020 15:24

remove migration from data source and resolve a self reference

99fab3d

fix build error...

0fd7b9d

tombuildsstuff suggested changes Aug 25, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Aug 25, 2020

PIT copy paste the v0 schema

94505f1

magodo removed the waiting-response label Aug 26, 2020

lint error

2129111

tombuildsstuff changed the title ~~azurerm_cnd_endpoint - Standardize the resource ID to be case sensitive~~ azurerm_cdn_endpoint - Standardize the resource ID to be case sensitive Aug 27, 2020

tombuildsstuff suggested changes Aug 27, 2020

View reviewed changes

tombuildsstuff added this to the v2.26.0 milestone Aug 27, 2020

PIT C/P cdn profile schema & expand all function and remove validatio…

cc631d1

…n in V0 schema

ghost removed the size/XL label Aug 27, 2020

ghost added the size/XXL label Aug 27, 2020

tombuildsstuff reviewed Aug 28, 2020

View reviewed changes

azurerm/internal/services/cdn/parse/cdn_profile.go Outdated Show resolved Hide resolved

azurerm/internal/services/cdn/parse/cdn_endpoint.go Outdated Show resolved Hide resolved

tombuildsstuff added the waiting-response label Aug 28, 2020

rename param for subscriptionId to be consistent

5ddc3d5

magodo removed the waiting-response label Aug 31, 2020

fixing the build

dff6dac

tombuildsstuff approved these changes Sep 2, 2020

View reviewed changes

tombuildsstuff merged commit 08539df into hashicorp:master Sep 2, 2020

tombuildsstuff added a commit that referenced this pull request Sep 2, 2020

updating to include #8237

4b4697f

ghost locked as resolved and limited conversation to collaborators Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_cdn_endpoint` - Standardize the resource ID to be case sensitive #8237

`azurerm_cdn_endpoint` - Standardize the resource ID to be case sensitive #8237

magodo commented Aug 25, 2020 •

edited

Loading

tombuildsstuff left a comment

magodo commented Aug 25, 2020

tombuildsstuff left a comment

tombuildsstuff left a comment

magodo commented Aug 25, 2020

tombuildsstuff left a comment

tombuildsstuff Aug 25, 2020

magodo Aug 26, 2020

tombuildsstuff Aug 26, 2020

tombuildsstuff Aug 25, 2020

magodo commented Aug 26, 2020

tombuildsstuff left a comment

magodo commented Aug 27, 2020

tombuildsstuff left a comment

magodo commented Aug 31, 2020

tombuildsstuff left a comment

tombuildsstuff commented Sep 2, 2020

ghost commented Sep 4, 2020

ghost commented Oct 2, 2020

azurerm_cdn_endpoint - Standardize the resource ID to be case sensitive #8237

azurerm_cdn_endpoint - Standardize the resource ID to be case sensitive #8237

Conversation

magodo commented Aug 25, 2020 • edited Loading

Test Result

tombuildsstuff left a comment

Choose a reason for hiding this comment

magodo commented Aug 25, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

magodo commented Aug 25, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Aug 25, 2020

Choose a reason for hiding this comment

magodo Aug 26, 2020

Choose a reason for hiding this comment

tombuildsstuff Aug 26, 2020

Choose a reason for hiding this comment

tombuildsstuff Aug 25, 2020

Choose a reason for hiding this comment

magodo commented Aug 26, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

magodo commented Aug 27, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

magodo commented Aug 31, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Sep 2, 2020

ghost commented Sep 4, 2020

ghost commented Oct 2, 2020

`azurerm_cdn_endpoint` - Standardize the resource ID to be case sensitive #8237

`azurerm_cdn_endpoint` - Standardize the resource ID to be case sensitive #8237

magodo commented Aug 25, 2020 •

edited

Loading