`azurerm_snapshot` - switch to go-azure-sdk #18957

myc2h6o · 2022-10-25T02:38:14Z

Updating resource and data source azurerm_snapshot to use go-azure-sdk.
Similar to what we have done for azurerm_managed_disk in azurerm_managed_disk - add state upgrader from V0 to V1 to fix ID issue #18885, snapshot requires a state migration function as well to resolve the enhanced ID validation of Microsoft.Compute vs Microsoft.compute in the new sdk.

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments inline, but this is off to a good start - if we can fix those up then we should be able to take another look and get this merged.

Thanks!

tombuildsstuff · 2022-10-25T11:45:32Z

internal/services/compute/migration/snapshot_v0_to_v1.go

+			Computed: true,
+		},
+
+		"encryption_settings": encryptionSettingsSchema(),


this is a point-in-time reference so needs to be in-lined else it'll fall out of sync

Understood, ~~I've copied the schema to this file. As the schema function checks feature flag, I'm still putting it in the function snapshotEncryptionSettingsSchemaV0~~

--Update
I've removed the feature flag in it and copy the schema inline per #18993 (comment)

tombuildsstuff · 2022-10-25T11:46:13Z

internal/services/compute/migration/snapshot_v0_to_v1.go

+			return rawState, err
+		}
+
+		rawState["id"] = oldId.ID()


we should log the new ID:

Suggested change

rawState["id"] = oldId.ID()

newId := oldId.ID()

log.Printf("[DEBUG] Updating the ID from %q to %q", oldIdRaw, newId)

rawState["id"] = newId

tombuildsstuff · 2022-10-25T11:46:24Z

internal/services/compute/migration/snapshot_v0_to_v1.go

+
+func (SnapshotV0ToV1) UpgradeFunc() pluginsdk.StateUpgraderFunc {
+	return func(ctx context.Context, rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
+		oldId, err := snapshots.ParseSnapshotIDInsensitively(rawState["id"].(string))


Suggested change

oldId, err := snapshots.ParseSnapshotIDInsensitively(rawState["id"].(string))

oldIdRaw := rawState["id"].(string)

oldId, err := snapshots.ParseSnapshotIDInsensitively(oldIdRaw)

tombuildsstuff · 2022-10-25T11:46:59Z

internal/services/compute/shared_image_version_resource_test.go

-	future, err := client.Compute.SnapshotsClient.RevokeAccess(ctx, resourceGroup, snapShotName)
-	if err != nil {
+	snapshotId := snapshots.NewSnapshotID(subscriptionId, resourceGroup, snapShotName)
+	if err := client.Compute.SnapshotsClient.RevokeAccessThenPoll(ctx, snapshotId); err != nil {
 		return fmt.Errorf("bad: cannot revoke SAS on the snapshot: %+v", err)


Suggested change

return fmt.Errorf("bad: cannot revoke SAS on the snapshot: %+v", err)

return fmt.Errorf("revoking SAS on %s: %+v", snapshotId, err)

tombuildsstuff · 2022-10-25T11:53:08Z

internal/services/compute/snapshot_data_source.go

+			if props.OsType != nil {
+				d.Set("os_type", string(*props.OsType))
+			}


this should always have a value:

Suggested change

if props.OsType != nil {

d.Set("os_type", string(*props.OsType))

}

osType := ""

if props.OsType != nil {

osType = string(*props.OsType)

}

d.Set("os_type", osType)

tombuildsstuff · 2022-10-25T12:23:16Z

internal/services/compute/snapshot_resource.go

+			if accountId := data.StorageAccountId; accountId != nil {
 				d.Set("storage_account_id", accountId)
 			}


(as above) this can always be set

tombuildsstuff · 2022-10-25T12:23:20Z

internal/services/compute/snapshot_resource.go

+			if props.DiskSizeGB != nil {
+				d.Set("disk_size_gb", int(*props.DiskSizeGB))
+			}


(as above) this can always be set

tombuildsstuff · 2022-10-25T12:23:43Z

internal/services/compute/snapshot_resource.go

+				if *securityProfile.SecurityType == snapshots.DiskSecurityTypesTrustedLaunch {
+					trustedLaunchEnabled = true
+				}


(as above) this can be reduced to:

Suggested change

if *securityProfile.SecurityType == snapshots.DiskSecurityTypesTrustedLaunch {

trustedLaunchEnabled = true

}

trustedLaunchEnabled = *securityProfile.SecurityType == snapshots.DiskSecurityTypesTrustedLaunch

tombuildsstuff · 2022-10-25T12:23:56Z

internal/services/compute/snapshot_resource.go

-	}
-
-	if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
+	if err := client.DeleteThenPoll(ctx, *id); err != nil {
 		return fmt.Errorf("deleting Snapshot: %+v", err)


Suggested change

return fmt.Errorf("deleting Snapshot: %+v", err)

return fmt.Errorf("deleting %s: %+v", *id, err)

tombuildsstuff · 2022-10-25T12:24:48Z

internal/services/compute/migration/snapshot_v0_to_v1.go

+		rawState["id"] = oldId.ID()
+		return rawState, nil
+	}
+}


can we also remove the old Resource ID Type from resourceids.go and the generated ID Parsers/Validators?

myc2h6o · 2022-10-26T05:10:39Z

Hi @tombuildsstuff thanks for reviewing the pr! I've updated the pr to resolve your comments, please take a look.

katbyte

LGTM 🎵

github-actions · 2022-10-28T00:12:33Z

This functionality has been released in v3.29.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2022-11-27T02:23:44Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added the dependencies label Oct 25, 2022

myc2h6o force-pushed the snapshot_sdk branch from 0d0d644 to 6cad386 Compare October 25, 2022 02:55

azurerm_snapshot - switch to go-azure-sdk

6fb067a

myc2h6o force-pushed the snapshot_sdk branch from 6cad386 to 6fb067a Compare October 25, 2022 03:19

tombuildsstuff suggested changes Oct 25, 2022

View reviewed changes

github-actions bot added the waiting-response label Oct 25, 2022

Resolve comments

3fdf336

github-actions bot removed the waiting-response label Oct 26, 2022

Remove snapshot parse

654a8b5

This was referenced Oct 26, 2022

disks - remove old parse and validate #18990

Merged

azurerm_managed_disk - add log and remove feature flag in state migration #18993

Merged

remove feature flag in state migration schema

aee2bd9

katbyte approved these changes Oct 26, 2022

View reviewed changes

katbyte merged commit 2034046 into hashicorp:main Oct 26, 2022

katbyte added a commit that referenced this pull request Oct 26, 2022

CHANGELOG #18957

d66bad1

github-actions bot added this to the v3.29.0 milestone Oct 26, 2022

myc2h6o deleted the snapshot_sdk branch October 27, 2022 02:04

github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_snapshot` - switch to go-azure-sdk #18957

`azurerm_snapshot` - switch to go-azure-sdk #18957

myc2h6o commented Oct 25, 2022 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Oct 25, 2022

myc2h6o Oct 26, 2022 •

edited

Loading

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

tombuildsstuff Oct 25, 2022

myc2h6o commented Oct 26, 2022

katbyte left a comment

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 27, 2022

-		rawState["id"] = oldId.ID()
+		newId := oldId.ID()
+		log.Printf("[DEBUG] Updating the ID from %q to %q", oldIdRaw, newId)
+		rawState["id"] = newId

	oldId, err := snapshots.ParseSnapshotIDInsensitively(rawState["id"].(string))
	oldIdRaw := rawState["id"].(string)
	oldId, err := snapshots.ParseSnapshotIDInsensitively(oldIdRaw)

	return fmt.Errorf("bad: cannot revoke SAS on the snapshot: %+v", err)
	return fmt.Errorf("revoking SAS on %s: %+v", snapshotId, err)

	return fmt.Errorf("deleting Snapshot: %+v", err)
	return fmt.Errorf("deleting %s: %+v", *id, err)

azurerm_snapshot - switch to go-azure-sdk #18957

azurerm_snapshot - switch to go-azure-sdk #18957

Conversation

myc2h6o commented Oct 25, 2022 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o commented Oct 26, 2022

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 27, 2022

`azurerm_snapshot` - switch to go-azure-sdk #18957

`azurerm_snapshot` - switch to go-azure-sdk #18957

myc2h6o commented Oct 25, 2022 •

edited

Loading

myc2h6o Oct 26, 2022 •

edited

Loading