[MS] Added ARM Image resource and made VM managed disks worked with custom images #14588

echuvyrov · 2017-05-17T18:00:09Z

The code includes a new ARM Image resource, which enables users to create VMs from custom image supplied in a storage account or from an existing VM (VM must be deprovisioned and generalized). Note that two acceptance tests currently require subscription-specific information (VHD has to be in a user account or VM must be generalized). This information would need to be provided by the person running the tests - open to ideas on improving this.

…er items based on review notes

…mage reference

stack72

Hi @echuvyrov

Thanks for the work here - left a few comments inline to have a think about before we merge this. It's very close though

Thanks

Paul

stack72 · 2017-05-18T10:08:26Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+// Place your settings in this file to overwrite default and user settings.


Please remove this file

stack72 · 2017-05-18T10:09:19Z

builtin/providers/azurerm/resource_arm_image.go

+			},
+
+			"os_disk": {
+				Type:     schema.TypeSet,


As there is only 1 of these allowed, I don't believe this needs to be a Set

stack72 · 2017-05-18T10:11:03Z

builtin/providers/azurerm/resource_arm_image.go

+
+func flattenAzureRmSourceVMProperties(d *schema.ResourceData, properties *compute.SubResource) {
+	if properties.ID != nil {
+		d.Set("source_virtual_machine_id", *properties.ID)


You don't need to dereference - d.Set does this in a safe manner. Right now, if ID was nil, this would panic

stack72 · 2017-05-18T10:11:51Z

builtin/providers/azurerm/resource_arm_image.go

+	osDisk := &compute.ImageOSDisk{}
+	disks := d.Get("os_disk").(*schema.Set).List()
+
+	if len(disks) > 0 {


wouldn't a range work here? We could loop over the items in the list - if it's empty then it just passes through

stack72 · 2017-05-18T11:05:37Z

builtin/providers/azurerm/resource_arm_virtual_machine.go

@@ -1370,18 +1394,35 @@ func expandAzureRmVirtualMachineImageReference(d *schema.ResourceData) (*compute
 	storageImageRefs := d.Get("storage_image_reference").(*schema.Set).List()

 	storageImageRef := storageImageRefs[0].(map[string]interface{})
+	imageReference := compute.ImageReference{}
+	imageID := ""


I don't think imageID and publisher need to be initialised here

stack72 · 2017-05-18T11:06:51Z

builtin/providers/azurerm/resource_arm_virtual_machine.go

-		Version:   &version,
-	}, nil
+	//BEGIN: code to be removed after GH-13016 is merged
+	if imageID != "" && publisher != "" {


Can this ever be true? You are setting publisher and imageID to "" - then using an if / else to hydrate the struct.

stack72 · 2017-05-18T11:08:18Z

builtin/providers/azurerm/resource_arm_image_test.go

+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAccAzureRMManagedImage_standaloneImage(t *testing.T) {


I'd like to see an Import test to show that functionality works as expected

…s/terraform into manageddiskcustomimage2

echuvyrov · 2017-05-19T05:58:15Z

@stack72 I made the changes requested and updated the docs. Note that the import test still relies on a pre-existing .vhd, which I made available publicly but that test would still fail for subscriptions other than mine. Not sure how I can work around this: per this doc, the image can only be created "using the specified blob, snapshot, managed disk, or existing virtual machine."

tombuildsstuff

Hi @echuvyrov

Thanks for pushing those updates - I've reviewed and left some comments in-line - but this is looking good :)

Regarding the image - given we need these tests to pass in any subscription, rather than relying on long-running resources I'd suggest it'd be better to spin up a VM and generalise it as part of the test - what do you think?

Thanks!

tombuildsstuff · 2017-05-26T18:39:24Z

builtin/providers/azurerm/provider.go

@@ -88,6 +88,7 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_lb_rule":                 resourceArmLoadBalancerRule(),

 			"azurerm_managed_disk": resourceArmManagedDisk(),
+			"azurerm_image":        resourceArmManagedImage(),


minor given this is an image and not an managed image could we rename the class to match?

tombuildsstuff · 2017-05-26T18:39:51Z

builtin/providers/azurerm/resource_arm_image.go

+	return &schema.Resource{
+		Create: resourceArmManagedImageCreate,
+		Read:   resourceArmManagedImageRead,
+		Update: resourceArmManagedImageCreate,


minor: given this is both Creating and Updating - we normally name this CreateUpdate

tombuildsstuff · 2017-05-26T18:40:52Z

builtin/providers/azurerm/resource_arm_image.go

+				ValidateFunc: validation.StringInSlice([]string{
+					string(compute.Linux),
+					string(compute.Windows),
+				}, true),


given this is true (for case insensitivity) I believe this wants a DiffSuppressFunc here to ignore the casing?

tombuildsstuff · 2017-05-26T18:41:10Z

builtin/providers/azurerm/resource_arm_image.go

+								string(compute.None),
+								string(compute.ReadOnly),
+								string(compute.ReadWrite),
+							}, true),


given this is true (for case insensitivity) I believe this wants a DiffSuppressFunc here to ignore the casing?

(same question about default value here)

tombuildsstuff · 2017-05-26T18:43:42Z

builtin/providers/azurerm/resource_arm_image.go

+		osDisk.OsType = osType
+	}
+
+	if v := d.Get("os_disk_os_state").(string); v != "" {


minor: given there's validation on this - I believe we can make this simpler:

if v, ok := d.Get("os_disk_os_state").(string); ok {

tombuildsstuff · 2017-06-01T12:18:14Z

website/source/docs/import/importability.html.md

@@ -121,6 +121,7 @@ To make a resource importable, please see the
 * azurerm_sql_firewall_rule
 * azurerm_storage_account
 * azurerm_virtual_network
+* azurerm_image


minor: these are currently sorted alphabetically for easier reading - could we re-order this? :)

tombuildsstuff · 2017-06-01T12:19:36Z

website/source/docs/providers/azurerm/r/image.html.markdown

+* `os_disk_managed_disk_id` - (Optional) Specifies the ID of the managed disk resource that you want to use to create the image.
+* `os_disk_blob_uri` - (Optional) Specifies the URI in Azure storage of the blob that you want to use to create the image.
+* `os_disk_caching` - (Optional) Specifies the caching mode as 'readonly', 'readwrite', or 'none'. The default is none.
+* `os_disk_size_gb` - (Optional) Specifies the size of the image to be created. The target size can't be smaller than the source size.


rather than prefixing all of these with os_disk - would it make more sense to put them in an os_disk block/hash?

tombuildsstuff · 2017-06-01T12:20:36Z

website/source/layouts/azurerm.erb

+            <li<%= sidebar_current("docs-azurerm-resource-managed-image") %>>
+              <a href="#">Managed Image Resources</a>
+              <ul class="nav nav-visible">
+                <li<%= sidebar_current("docs-azurerm-resource-managed-image") %>>


minor can we remove the word managed from this section?

tombuildsstuff · 2017-06-01T12:21:07Z

website/source/docs/providers/azurerm/r/image.html.markdown

+  Create a custom virtual machine image that can be used to create virtual machines.
+---
+
+# azurerm\_managed\_image


I think this should be azurerm_image?

tombuildsstuff · 2017-06-01T12:21:12Z

website/source/docs/providers/azurerm/r/image.html.markdown

+---
+layout: "azurerm"
+page_title: "Azure Resource Manager: azurerm_image"
+sidebar_current: "docs-azurerm-resource-managed-image"


minor can we remove the word managed from this?

echuvyrov · 2017-06-06T08:16:54Z

@tombuildsstuff I made the changes requested, and the tests should now be fully self contained as per your suggestion (I stand up a VM, deprovision/generalize it, then grab the .vhd or simply point to that VM).

Regarding making "os_disk" a separate block - I went that route during the initial implementation, but @stack72 pointed out that since there will always be just one "os_disk" resource, it should be at the root level. Let me know if I misunderstood and thanks for your timely and thorough reviews.

…image2

echuvyrov · 2017-06-13T04:05:23Z

Moved here per 0.10 guidelines

stack72 · 2017-06-13T07:11:39Z

Thanks so much @echuvyrov :)

ghost · 2020-04-11T02:14:17Z

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.

echuvyrov added 9 commits May 15, 2017 16:14

Managed Disks with Custom Image Initial Work

3bb2c6a

Managed Disks with Custom Image Initial Work - 2

03e2b28

Managed Disks with Custom Image - Ready for review for VMs

1fe050d

Managed Disks with Custom Image - Ready for review for VMs

c34a9cd

Changes requested by code reviewers and documentation added

1226980

Changes to Images doc

8efa264

Renamed managed_image to image, corrected state issues, corrected oth…

51a9720

…er items based on review notes

Changes to ensure state is maintained properly for VMs with the new i…

33e0a02

…mage reference

Changed blob URI in tests

7a6dbd1

grubernaut added enhancement new-resource provider/azurerm labels May 17, 2017

stack72 suggested changes May 18, 2017

View reviewed changes

stack72 added the waiting-response An issue/pull request is waiting for a response from the community label May 18, 2017

echuvyrov added 3 commits May 18, 2017 22:43

Changes to custom image with Managed Disks

181f458

Changes to custom image with Managed Disks

a92ebd4

Merge branch 'manageddiskcustomimage2' of https://github.com/harijaym…

35607ea

…s/terraform into manageddiskcustomimage2

tombuildsstuff removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 1, 2017

tombuildsstuff reviewed Jun 1, 2017

View reviewed changes

echuvyrov added 2 commits June 1, 2017 12:05

Adding additional tests

e02cca6

Made changes requested to tests and code

13d13e0

echuvyrov added 2 commits June 6, 2017 11:26

Merge remote-tracking branch 'upstream/master' into manageddiskcustom…

426abaf

…image2

Made changes related to newer Go SDK

15fc8df

azdevops mentioned this pull request Jun 7, 2017

hashicorp#14588 I can use managed disks with custom images azdevops/terraform#8

Closed

stack72 closed this Jun 13, 2017

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MS] Added ARM Image resource and made VM managed disks worked with custom images #14588

[MS] Added ARM Image resource and made VM managed disks worked with custom images #14588

echuvyrov commented May 17, 2017

stack72 left a comment

stack72 May 18, 2017

stack72 May 18, 2017

stack72 May 18, 2017

stack72 May 18, 2017

stack72 May 18, 2017

stack72 May 18, 2017

stack72 May 18, 2017

echuvyrov commented May 19, 2017

tombuildsstuff left a comment

tombuildsstuff May 26, 2017

tombuildsstuff May 26, 2017

tombuildsstuff May 26, 2017

tombuildsstuff May 26, 2017

tombuildsstuff Jun 1, 2017

tombuildsstuff May 26, 2017

tombuildsstuff Jun 1, 2017

tombuildsstuff Jun 1, 2017

tombuildsstuff Jun 1, 2017

tombuildsstuff Jun 1, 2017

tombuildsstuff Jun 1, 2017

echuvyrov commented Jun 6, 2017

echuvyrov commented Jun 13, 2017

stack72 commented Jun 13, 2017

ghost commented Apr 11, 2020

		@@ -0,0 +1,3 @@
		// Place your settings in this file to overwrite default and user settings.

[MS] Added ARM Image resource and made VM managed disks worked with custom images #14588

[MS] Added ARM Image resource and made VM managed disks worked with custom images #14588

Conversation

echuvyrov commented May 17, 2017

stack72 left a 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

echuvyrov commented May 19, 2017

tombuildsstuff left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echuvyrov commented Jun 6, 2017

echuvyrov commented Jun 13, 2017

stack72 commented Jun 13, 2017

ghost commented Apr 11, 2020