Add name_regex to azurerm_image data source #642

paultyng · 2017-12-19T16:35:03Z

Fixes #454

paultyng · 2017-12-19T16:48:53Z

azurerm/data_source_image_test.go

+data "azurerm_image" "test2" {
+  name_regex          = "^[a-z]+-acctest-\\d+"
+  name_descending     = true
+  resource_group_name = "${azurerm_resource_group.test.name}${substr(azurerm_image.abc.name, 0, 0)}${substr(azurerm_image.def.name, 0, 0)}"


This was a hack to deal with depends_on and data source issues, is there a better way to do this?

alas not really - some tests create the config in one step then apply the data source in another; I think there's a core bug about that?

tombuildsstuff

Hey @paultyng

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start.

My main question is if it's worth mirroring the behaviour of the AWS AMI Data Source - in that, if multiple images are specified should we fail, rather than picking the default?

Thanks!

tombuildsstuff · 2017-12-19T16:49:54Z

azurerm/data_source_image.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,
+				ValidateFunc: validateNameRegex,


we should probably add a ConflictsWith attribute to this for the name field?

I might be wrong - but I think there's a helper that does this in the validation namespace?

tombuildsstuff · 2017-12-19T16:49:58Z

azurerm/data_source_image.go

 			"name": {
 				Type:     schema.TypeString,
-				Required: true,
+				Optional: true,


we should probably add a ConflictsWith attribute to this for the name_regex field?

tombuildsstuff · 2017-12-20T18:32:03Z

website/docs/d/image.html.markdown

-* `name` - (Required) The name of the Image.
+* `name` - (Optional) The name of the Image.
+* `name_regex` - (Optional) Regex pattern of the image to match.
+* `name_descending` - (Optional) By default when matching by regex, images are sorted by name in ascending order and the first match is chosen, to sort descending, set this flag.


would it be instead worth making this a most_recent flag as in the AWS data source?

tombuildsstuff · 2017-12-20T18:32:45Z

azurerm/data_source_image_test.go

+data "azurerm_image" "test2" {
+  name_regex          = "^[a-z]+-acctest-\\d+"
+  name_descending     = true
+  resource_group_name = "${azurerm_resource_group.test.name}${substr(azurerm_image.abc.name, 0, 0)}${substr(azurerm_image.def.name, 0, 0)}"


alas not really - some tests create the config in one step then apply the data source in another; I think there's a core bug about that?

tombuildsstuff · 2017-12-20T18:33:37Z

azurerm/data_source_image_test.go

+  os_disk {
+    os_type  = "Linux"
+    os_state = "Generalized"
+    blob_uri = "${azurerm_storage_account.test.primary_blob_endpoint}${azurerm_storage_container.test.name}/myosdisk1.vhd"


I think we should be able to make this ${azurerm_virtual_machine.storage_os_disk.0.blob_uri}?

tombuildsstuff · 2017-12-20T18:33:54Z

azurerm/data_source_image_test.go

+  os_disk {
+    os_type  = "Linux"
+    os_state = "Generalized"
+    blob_uri = "${azurerm_storage_account.test.primary_blob_endpoint}${azurerm_storage_container.test.name}/myosdisk1.vhd"


I think we should be able to make this ${azurerm_virtual_machine.storage_os_disk.0.blob_uri} - which means we could remove the depends_on?

possibly, the bulk of this test was just copied from the existing one, I will update / test

tombuildsstuff · 2017-12-20T18:42:21Z

azurerm/data_source_image.go

+
+		if len(list) > 1 {
+			desc := d.Get("name_descending").(bool)
+			log.Printf("[DEBUG] arm_image - multiple results found and `name_descending` is set to: %t", desc)


for the AWS provider we fail in this case unless most_recent is specified - is it worth mirroring that behaviour?

if we had the ability to do most recent i would agree and would have just matched AWS, without that, since we are sorting by name, either ascending or descending, we may need an additional attribute to specify if a single match is expected, but we could do that. I had assumed that if you are using the regex you intend to match from a list, but that could be an bad assumption.

tombuildsstuff · 2017-12-20T18:44:47Z

azurerm/data_source_image.go

+			desc := d.Get("name_descending").(bool)
+			log.Printf("[DEBUG] arm_image - multiple results found and `name_descending` is set to: %t", desc)
+
+			sort.Slice(list, func(i, j int) bool {


it'd be good to add an acceptance test for this behaviour too?

paultyng · 2017-12-20T19:34:13Z

Maybe its worth just waiting until they surface created date on the images?

Alternatively we could possibly infer created date via the blobs? It would be very chatty, and kind of a hack, but could use the latest date of the disk blobs?

paultyng · 2018-01-05T15:05:02Z

tombuildsstuff

hey @paultyng

Sorry for the delayed re-review of this - taking a look through this LGTM - the final thing before merging this would be to rename the name_descending field to sort_descending to make it clearer what it does (and also support sorting by additional fields in the future). Other than that this LGTM :)

Thanks!

tombuildsstuff · 2018-01-15T12:05:05Z

azurerm/data_source_image.go

+				ValidateFunc:  validation.ValidateRegexp,
+				ConflictsWith: []string{"name"},
+			},
+			"name_descending": {


after chatting with @radeksimko - can we rename this sort_descending? it's possible we'll filter by additional fields in the future

Fixes #454

paultyng · 2018-01-18T15:40:13Z

=== RUN   TestAccDataSourceAzureRMImage_basic
--- PASS: TestAccDataSourceAzureRMImage_basic (492.36s)
=== RUN   TestAccDataSourceAzureRMImage_localFilter
--- PASS: TestAccDataSourceAzureRMImage_localFilter (528.13s)

tombuildsstuff

hey @paultyng

Thanks for pushing those changes - I've pushed a commit to update the field name in the documentation - but this now LGTM 👍

Thanks!

ghost · 2020-04-01T14:52:03Z

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!

paultyng commented Dec 19, 2017

View reviewed changes

paultyng requested a review from tombuildsstuff December 19, 2017 16:54

tombuildsstuff added the enhancement label Dec 20, 2017

tombuildsstuff suggested changes Dec 20, 2017

View reviewed changes

tombuildsstuff added this to the 1.0.2 milestone Jan 10, 2018

tombuildsstuff approved these changes Jan 15, 2018

View reviewed changes

paultyng added 2 commits January 18, 2018 10:19

Add name_regex to azurerm_image data source

a82f6db

Fixes #454

Fix unnecessary depends_on

c2ef5e4

tombuildsstuff added 2 commits January 18, 2018 15:46

Updating the field name in the documentation to sort_descending

5761db3

Updating the field name in the documentation to sort_descending

9128634

tombuildsstuff approved these changes Jan 18, 2018

View reviewed changes

tombuildsstuff merged commit ebc6a8f into hashicorp:master Jan 18, 2018

tombuildsstuff added a commit that referenced this pull request Jan 18, 2018

Updating to include #642

d3d5b85

paultyng deleted the pt/add-name-regex branch January 18, 2018 15:53

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add name_regex to azurerm_image data source #642

Add name_regex to azurerm_image data source #642

paultyng commented Dec 19, 2017

paultyng Dec 19, 2017

tombuildsstuff Dec 20, 2017

tombuildsstuff left a comment

tombuildsstuff Dec 19, 2017

tombuildsstuff Dec 20, 2017

tombuildsstuff Dec 19, 2017

tombuildsstuff Dec 20, 2017

tombuildsstuff Dec 20, 2017

tombuildsstuff Dec 20, 2017

tombuildsstuff Dec 20, 2017

paultyng Dec 20, 2017

tombuildsstuff Dec 20, 2017

paultyng Dec 20, 2017

tombuildsstuff Dec 20, 2017

paultyng commented Dec 20, 2017

paultyng commented Jan 5, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 15, 2018

paultyng commented Jan 18, 2018

tombuildsstuff left a comment

ghost commented Apr 1, 2020

Add name_regex to azurerm_image data source #642

Add name_regex to azurerm_image data source #642

Conversation

paultyng commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

paultyng commented Dec 20, 2017

paultyng commented Jan 5, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultyng commented Jan 18, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

ghost commented Apr 1, 2020