Add the "google_compute_image" datasource #764

eilgin · 2017-11-17T19:43:12Z

We can't currently use custom images from other projects which is quite helpful in order to make "immutable" servers especially with the "family image" feature where we get the latest generated image (e.g. using packer) from a dedicated project.

Acceptance result from this datasource:

TF_ACC=1 go test $(go list ./... | grep -v 'vendor') -v -run=TestAccDataSourceComputeImage -timeout 120m
?       github.com/terraform-providers/terraform-provider-google        [no test files]
=== RUN   TestAccDataSourceComputeImage
--- PASS: TestAccDataSourceComputeImage (70.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 70.304s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-google/scripts        0.022s [no tests to run]

The refactored "image" still pass the acceptance tests:

TF_ACC=1 go test $(go list ./... | grep -v 'vendor') -v -run=TestAccComputeImage_resolveImage -timeout 120m
?       github.com/terraform-providers/terraform-provider-google        [no test files]
=== RUN   TestAccComputeImage_resolveImage
--- PASS: TestAccComputeImage_resolveImage (79.09s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 79.110s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-google/scripts        0.031s [no tests to run]

This closes #128.

rosbo · 2017-11-17T20:51:57Z

@paddycarver Are you still working on a data source for image?

eilgin · 2017-11-27T11:28:14Z

Are there any updates on the status of this PR ?

paddycarver

Some preliminary feedback here. I'm mostly concerned about the design and the ergonomics of it, especially the the part about not being able to disable the project override on any image name that matches a set of semi-common strings, and the part about conflating family and image.

I dunno if it's helpful, but I do have a ~code complete version of this data source I've pushed here: https://github.com/terraform-providers/terraform-provider-google/blob/paddy_128_image_data_source/google/data_source_compute_image.go Feel free to use that for inspiration, or if you'd prefer, I'm happy to merge your documentation over to it and reuse the code and push forward with that one. I'm not wed to the code, though, so if you'd rather use your implementation as a base and continue iterating on it, I'm happy to do that, as well. :)

Thanks for contributing to Terraform, and my apologies for the long review delay!

paddycarver · 2017-11-29T15:39:04Z

google/image.go

@@ -27,8 +28,12 @@ var (
 	resolveImageLink                   = regexp.MustCompile(fmt.Sprintf("^https://www.googleapis.com/compute/v1/projects/(%s)/global/images/(%s)", resolveImageProjectRegex, resolveImageImageRegex))
 )

+func fetchImage(c *Config, project, name string) (*compute.Image, error) {
+	return c.clientCompute.Images.Get(project, name).Do()
+}


I'm not super-wild about the helper functions that wrap a single function call. This feels like it doesn't really add value over just calling the API directly. This definitely isn't worth blocking merge over, but if edits are needed anyways, maybe worth fixing while fixing other stuff.

I agree on that too. No need to refactor one line of code into a function, i'll change that.

paddycarver · 2017-11-29T15:42:40Z

google/data_source_google_compute_image.go

+			"name": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,
+			},


Does it make more sense to make this optional, and have users enter names in the name attribute, or families in the family attribute? When we're specific like that, it can clear up some of the ambiguity.

If you choose to declare a family it makes more sense.

paddycarver · 2017-11-29T15:48:07Z

google/data_source_google_compute_image.go

+	builtInProject := findProjectFromBuiltinImage(name)
+
+	if builtInProject != "" {
+		project = builtInProject


As a user, how would I go about specifying an image of my own that shared a name with an image that has a builtin project? The way this is written, I think any image that has a family or image name containing "centos", "coreos", "debian", "opensuse", "rhel", "sles", "ubuntu", or "windows" will override the user-specified project with a hardcoded one, which will often not be the right thing to do, and I can't see a way around that behaviour at the moment. I see two potential fixes for this (there may be more):

Establish a hierarchy. Project is an optional field, so we could check if project is empty, fall back on the built in project list if it is, and fall back on the provider project if we still don't know what to use.

Make it explicit. Rather than inferring these built-in projects, users could just set the project for them, themselves.

I agree with you on the second solution: making it explicit when choosing an image even a builtin one.

paddycarver · 2017-11-29T15:49:11Z

google/data_source_google_compute_image.go

+	d.Set("size", image.DiskSizeGb)
+	d.Set("self_link", image.SelfLink)
+	d.Set("project", project)
+	d.SetId(image.Name)


What happens if two resources are created with name the same but project different? Terraform would get confused, I think. We should probably embed the project in the ID, to make sure the ID is actually unique.

You're right. i've noticed that you've used the ${family}/${name} and forgot to prefix it with the project ID, i'll fix that also :)

eilgin · 2017-12-04T19:56:09Z

If you don't mind, i'll merge your solution with mine and update this PR accordingly.

paddycarver · 2017-12-04T20:00:02Z

Awesome! Looking forward to it. Excited to see this make it into the provider, and I'm really glad we got a chance to collaborate--I think merging our two approaches and having two sets of eyes on it will give us a really well-designed data source that works well for people. Thanks so much for the patience on this.

eilgin · 2017-12-04T23:43:16Z

Changes done. Acceptance tests are working:

TF_ACC=1 go test $(go list ./... | grep -v 'vendor') -v -run=TestAccDataSourceComputeImage -timeout 120m
?       github.com/terraform-providers/terraform-provider-google        [no test files]
=== RUN   TestAccDataSourceComputeImage
--- PASS: TestAccDataSourceComputeImage (55.59s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 55.619s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-google/scripts        0.028s [no tests to run]

paddycarver

Two incredibly minor documentation updates, and this looks good! If you don't get to them by Monday, I'll just take care of them. :)

paddycarver · 2017-12-07T22:13:40Z

website/docs/d/datasource_compute_image.html.markdown

+
+```hcl
+data "google_compute_image" "my_image" {
+  name = "image-family"


This seems misleading. Let's either do family = "image-family" or name = "image-name"

paddycarver · 2017-12-07T22:18:36Z

website/docs/d/datasource_compute_image.html.markdown

+
+* `self_link` - The URI of the resource.
+* `name` - The image name of the resource in case `family` was specified.
+* `family` - The family name of the resource in case `name` was specified.


These should both be set, regardless of which was specified, right?

paddycarver · 2017-12-08T22:12:37Z

I just pushed the doc fixes for this to a new branch, opened a new PR, and merged it, but keeping your commits intact. I didn't want your great work to get left out of another release over some documentation wording changes.

Thanks so much for the patience and contributing this feature!

eilgin · 2017-12-10T23:10:18Z

i should definitively re-activate the notification!

Thank you so much for your thorough review and your hints! It's great to see this datasource coming to the next release, it'll be really handy for my team to have that :)

paddycarver · 2017-12-11T00:57:48Z

No worries at all, and sorry for jumping the gun on Friday instead of Monday. Please rest assured it is not a critique of your capabilities or timeliness, just a way to expedite this and make sure another release doesn't go out without it. :)

Thanks so much for all the work you put into this, I think it's an amazing feature to add and something that has been on my backburner for a long, long time, and I'm really grateful you got to it and got it done.

rvaidya · 2017-12-12T05:20:22Z

Thanks a lot for adding this!

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost · 2020-03-30T14:05:14Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

rosbo requested a review from paddycarver November 17, 2017 20:51

paddycarver suggested changes Nov 29, 2017

View reviewed changes

thomas jouannic added 5 commits December 4, 2017 23:43

Add the "compute_image" datasource

95597bf

Add tests for the "compute_image" datasource

81bb363

Include the "compute_image" datasource in the provider

af3dcfd

Add documentation about the "compute_image" datasource

867a979

Include the "compute_image" datasource doc into the sidebar

0120a84

eilgin force-pushed the ds_compute_image branch from da3f34d to 0120a84 Compare December 4, 2017 23:40

paddycarver suggested changes Dec 7, 2017

View reviewed changes

paddycarver added enhancement new-data-source waiting-response labels Dec 7, 2017

paddycarver added this to the 1.4.0 milestone Dec 7, 2017

paddycarver mentioned this pull request Dec 8, 2017

Add compute image data source #833

Merged

paddycarver merged commit 0120a84 into hashicorp:master Dec 8, 2017

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

Replace description: > with description: | (hashicorp#764)

f6b6047

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the "google_compute_image" datasource #764

Add the "google_compute_image" datasource #764

eilgin commented Nov 17, 2017 •

edited

Loading

rosbo commented Nov 17, 2017

eilgin commented Nov 27, 2017

paddycarver left a comment

paddycarver Nov 29, 2017

eilgin Dec 4, 2017

paddycarver Nov 29, 2017

eilgin Dec 4, 2017

paddycarver Nov 29, 2017

eilgin Dec 4, 2017

paddycarver Nov 29, 2017

eilgin Dec 4, 2017 •

edited

Loading

eilgin commented Dec 4, 2017

paddycarver commented Dec 4, 2017

eilgin commented Dec 4, 2017

paddycarver left a comment

paddycarver Dec 7, 2017

paddycarver Dec 7, 2017

paddycarver commented Dec 8, 2017

eilgin commented Dec 10, 2017

paddycarver commented Dec 11, 2017

rvaidya commented Dec 12, 2017

ghost commented Mar 30, 2020

Add the "google_compute_image" datasource #764

Add the "google_compute_image" datasource #764

Conversation

eilgin commented Nov 17, 2017 • edited Loading

rosbo commented Nov 17, 2017

eilgin commented Nov 27, 2017

paddycarver 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

eilgin Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

eilgin commented Dec 4, 2017

paddycarver commented Dec 4, 2017

eilgin commented Dec 4, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver commented Dec 8, 2017

eilgin commented Dec 10, 2017

paddycarver commented Dec 11, 2017

rvaidya commented Dec 12, 2017

ghost commented Mar 30, 2020

eilgin commented Nov 17, 2017 •

edited

Loading

eilgin Dec 4, 2017 •

edited

Loading