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

Make google_notebooks_instance metadata field updatable #7945

Merged
merged 7 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions mmv1/products/notebooks/Instance.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ examples:
service_account: :SERVICE_ACCT
custom_code: !ruby/object:Provider::Terraform::CustomCode
constants: templates/terraform/constants/notebooks_instance.go
update_encoder: templates/terraform/update_encoder/notebooks_instance.go
parameters:
- !ruby/object:Api::Type::ResourceRef
name: 'location'
Expand Down Expand Up @@ -362,7 +363,11 @@ properties:
description: |
Custom metadata to apply to this instance.
An object containing a list of "key": value pairs. Example: { "name": "wrench", "mass": "1.3kg", "count": "3" }.
# This was added as part of https://github.com/GoogleCloudPlatform/magic-modules/pull/3761 to
# avoid detecting drift, since this field is partially server-managed.
ignore_read: true
update_verb: :PATCH
update_url: 'projects/{{project}}/locations/{{location}}/instances/{{name}}:updateMetadataItems'
- !ruby/object:Api::Type::Time
name: 'createTime'
description: 'Instance creation time'
Expand Down
7 changes: 7 additions & 0 deletions mmv1/templates/terraform/update_encoder/notebooks_instance.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Update requests use "items" as the api name instead of "metadata"
// https://cloud.google.com/vertex-ai/docs/workbench/reference/rest/v1/projects.locations.instances/updateMetadataItems
if metadata, ok := obj["metadata"]; ok {
obj["items"] = metadata
delete(obj, "metadata")
}
return obj, nil
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
<% autogen_exception -%>
package google

<% unless version == 'ga' %>

import (
"fmt"
"strconv"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
Expand Down Expand Up @@ -47,25 +46,25 @@ func TestAccNotebooksInstance_update(t *testing.T) {
ResourceName: "google_notebooks_instance.instance",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"vm_image"},
ImportStateVerifyIgnore: []string{"vm_image", "metadata"},
},
{
Config: testAccNotebooksInstance_update(context),
Config: testAccNotebooksInstance_update(context, true),
},
{
ResourceName: "google_notebooks_instance.instance",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"vm_image"},
ImportStateVerifyIgnore: []string{"vm_image", "metadata"},
},
{
Config: testAccNotebooksInstance_basic(context),
Config: testAccNotebooksInstance_update(context, false),
},
{
ResourceName: "google_notebooks_instance.instance",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"vm_image"},
ImportStateVerifyIgnore: []string{"vm_image", "metadata"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I understanding this correctly that we aren't ever checking that the metadata is updated on the server?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. The field is marked as ignore_read on the resource, so we never check what the value on the server is. I had to add it here because I introduced it as a new field in this test.

I suspect that we would not use ignore_read like this if implementing the resource today - but fixing that wouldn't be trivial and is outside of the scope of the ticket. In the future I think we'd want to do something similar to hashicorp/terraform-provider-google#14293.

},
},
})
Expand Down Expand Up @@ -108,34 +107,46 @@ resource "google_notebooks_instance" "instance" {
project = "deeplearning-platform-release"
image_family = "tf-latest-cpu"
}

metadata = {
proxy-mode = "service_account"
terraform = "true"
}

lifecycle {
prevent_destroy = true
}
}
`, context)
}

func testAccNotebooksInstance_update(context map[string]interface{}) string {
func testAccNotebooksInstance_update(context map[string]interface{}, preventDestroy bool) string {
context["prevent_destroy"] = strconv.FormatBool(preventDestroy)

return Nprintf(`
resource "google_notebooks_instance" "instance" {
name = "tf-test-notebooks-instance%{random_suffix}"
location = "us-central1-a"
machine_type = "e2-medium"

nic_type = "VIRTIO_NET"

reservation_affinity {
consume_reservation_type = "NO_RESERVATION"
}

vm_image {
project = "deeplearning-platform-release"
image_family = "tf-latest-cpu"
}

metadata = {
proxy-mode = "service_account"
terraform = "true"
notebook-upgrade-schedule = "0 * * * *"
}

labels = {
key = "value"
}

lifecycle {
prevent_destroy = %{prevent_destroy}
}
}
`, context)
}


<% end -%>