Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement 'licenses' field in compute_image resource #1717

Merged
merged 2 commits into from
Jul 12, 2018

Conversation

Mierdin
Copy link
Contributor

@Mierdin Mierdin commented Jun 28, 2018

This PR implements the licenses field for GCP images in this provider (already exists in the Google API, just needed to be plumbed through).

Terraform users can now use this provider to create images for nested virtualization, as well as other use cases for setting licenses on an image.

Closes #1045

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just a test update, but I think we could probably make licenses a Set, instead of a List.

@@ -29,6 +29,7 @@ func TestAccComputeImage_basic(t *testing.T) {
testAccCheckComputeImageFamily(&image, "family-test"),
testAccCheckComputeImageContainsLabel(&image, "my-label", "my-label-value"),
testAccCheckComputeImageContainsLabel(&image, "empty-label", ""),
testAccCheckComputeImageContainsLicense(&image, "https://www.googleapis.com/compute/v1/projects/vm-options/global/licenses/enable-vmx"),
Copy link
Contributor

@paddycarver paddycarver Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather see this in its own test, just so we can verify that creating an image without a license also works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup you're right, this was lazy. Updated in d7c1bf9

@@ -106,6 +106,13 @@ func resourceComputeImage() *schema.Resource {
Set: schema.HashString,
},

"licenses": &schema.Schema{
Type: schema.TypeList,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does order matter for licenses? Is it reasonable to expect someone to try and reference each individual license using interpolations?

Copy link
Contributor Author

@Mierdin Mierdin Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither the CLI or API docs seem to give an indication that order matters. Does your previous comment about using a Set instead of a List impact the order? I haven't delved into the details there yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is nice. So Google isn't expecting the order to be meaningful, but I'd say it might still be intuitive to keep this as a TypeList in case the user wants to be able to refer to it using interpolations. (I'm assuming they won't be able to do this if it's an unordered TypeSet). So not necessarily for compatibility reasons, but for UX reasons, might be useful to keep it as-is. Let me know if you disagree, I'm not married to it.

@paddycarver paddycarver merged commit 3d2e722 into hashicorp:master Jul 12, 2018
@Mierdin
Copy link
Contributor Author

Mierdin commented Jul 12, 2018

Thanks @paddycarver!

@paddycarver
Copy link
Contributor

Thank you! Apologies for the wait. I'm not sure how I feel about list vs set still, but we have a path forward if we decide we want to change it in the future, so let's ship it. :D

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
Implement 'licenses' field in compute_image resource
@ghost
Copy link

ghost commented Nov 17, 2018

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!

@ghost ghost locked and limited conversation to collaborators Nov 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants