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.

Sign up for GitHub

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

Jump to bottom

Add support for google_compute_project_metadata_item #176

Merged

selmanj merged 3 commits into hashicorp:master from selmanj:add_fine_grained_project_metadata

Jul 17, 2017

Contributor

selmanj commented Jul 5, 2017

This allows terraform users to manage single key/value items within the
project metadata map, rather than the entire map itself.

selmanj added the enhancement label

selmanj requested review from danawillow and rileykarson

July 5, 2017 19:49

Contributor Author

selmanj commented Jul 5, 2017 •

edited

Loading

Oops, forgot to gofmt

selmanj force-pushed the add_fine_grained_project_metadata branch 2 times, most recently from 6ebf6ec to f60453f Compare

July 5, 2017 19:55

rileykarson reviewed

View reviewed changes

website/docs/r/compute_project_metadata.html.markdown

@@ @@ -8,7 +8,10 @@ description: |- @@
               # google\_compute\_project\_metadata
-              Manages metadata common to all instances for a project in GCE.
+              Manages metadata common to all instances for a project in GCE. If you

Collaborator

rileykarson Jul 5, 2017

What if we state that this resource authoritatively manages all of the project's metadata before we explain google_compute_project_metadata_item as an alternative? That way it will be more clear what the potential gotchas with using this resource are, and how the alternative solves them.

Contributor Author

selmanj Jul 5, 2017

I almost documented exactly this, but unfortunately there's a bug right now in how google_compute_project_metadata works; right now it works similar to this PR, except upon delete (which deletes the entire map). We have a work item to go fix this so that it manages the entire metadata, but until then I'm hesitant to document that behavior (hence the current carefully worded description).

Collaborator

rileykarson Jul 6, 2017

SGTM then 👍

website/docs/r/compute_project_metadata.html.markdown Outdated

-              Manages metadata common to all instances for a project in GCE.
+              Manages metadata common to all instances for a project in GCE. If you
+              want to manage only single key/value pairs within the project metadata
+              rather than the entire map, then use

Collaborator

rileykarson Jul 5, 2017

Is it being a map an implementation detail? We describe it as a series below in the metadata field docs, for example.

Contributor Author

selmanj Jul 5, 2017

Hmm, good point. I see in the documentation about it being a set of metadata field docs, but nowhere about it being a map. Will update.

google/provider.go Outdated

               	}
               	return false
               }
+              func computeMetadataToMap(metadata []*compute.MetadataItems) map[string]string {

Collaborator

rileykarson Jul 5, 2017

We can name these functions flattenComputeMetadata (computeMetadataToMap) and expandComputeMetadata (mapToComputeMetadata) to match similar functions like in google_compute_backend_service.

Contributor Author

selmanj Jul 5, 2017

I can do that for consistency's sake, but I am not sure the naming is improved.

google/resource_compute_project_metadata_item.go Outdated

+              			State: schema.ImportStatePassthrough,
+              		},
+              		SchemaVersion: 0,

Collaborator

rileykarson Jul 5, 2017

We don't need to set a SchemaVersion until we perform a migration.

Contributor Author

selmanj Jul 5, 2017

Will remove

selmanj force-pushed the add_fine_grained_project_metadata branch 3 times, most recently from c4e7a03 to 407a312 Compare

July 6, 2017 18:27

danawillow suggested changes

View reviewed changes

google/resource_compute_project_metadata_item.go Outdated

+              		},
+              		Schema: map[string]*schema.Schema{
+              			"project": {

Contributor

danawillow Jul 6, 2017

nit: typically we put all required elements before all optional ones

Contributor Author

selmanj Jul 7, 2017

Fixed

google/resource_compute_project_metadata_item.go Outdated

+              	return nil
+              }
+              func updateComputeCommonInstanceMetadata(config *Config, projectID string, key string, beforeVal *string, afterVal *string) error {

Contributor

danawillow Jul 6, 2017

why *string?

Contributor

danawillow Jul 6, 2017

Oh is it because of differentiating between unset and set to the empty string? If so can you add a comment for clarity?

Contributor Author

selmanj Jul 7, 2017

Yes; will do

(ps do we have a maybe type or something better than *string)?

google/resource_compute_project_metadata_item.go Outdated

+              			return fmt.Errorf("Error loading project '%s': %s", projectID, err)
+              		}
+              		// Should the ID contain the project name?

Contributor

danawillow Jul 6, 2017

which id?

Contributor Author

selmanj Jul 7, 2017

This was supposed to have a TODO/NOTE in front of it but i foolishly forgot here - sorry, removing.

google/resource_compute_project_metadata_item.go Outdated

+              				// Asked to set no value and we didn't find one - we're done
+              				return nil
+              			}
+              			if beforeVal != nil {

Contributor

danawillow Jul 6, 2017

So the only example I can think of for us actually reaching this case is on a race condition where the key is deleted between the refresh and the apply. Either way, I'm not sure we necessarily need to error on it- if this is an update we can just add the new value, and if it's a delete we can just exit. Is there a different case you can think of?

Contributor Author

selmanj Jul 7, 2017

When I wrote this, it wasn't clear to me when a refresh occurred. Does it always occur before a write?

If so, then the race condition you listed above is the only one I can think of. If that's not worth checking then I'm not sure it's worth enforcing a beforeVal at all, right?

google/resource_compute_project_metadata_item_test.go Outdated

+              	})
+              }
+              func TestAccComputeProjectMetadataItem_basicImport(t *testing.T) {

Contributor

danawillow Jul 6, 2017

I don't have a good justification for this, but we tend to put import tests in their own file (and for consistency's sake it would be called [...]_importBasic)

Contributor Author

selmanj Jul 7, 2017

OK, will do

website/docs/r/compute_project_metadata_item.html.markdown Outdated

+              ---
+              layout: "google"
+              page_title: "Google: google_compute_project_metadata_item"
+              sidebar_current: "docs-google-compute-project-metadata_item"

Contributor

danawillow Jul 6, 2017

I think that last _ should be a -

Contributor Author

selmanj Jul 7, 2017

AGH, fixed

website/docs/r/compute_project_metadata_item.html.markdown Outdated

+              ```hcl
+              resource "google_compute_project_metadata_item" "default" {
+                key: "my_metadata"

Contributor

danawillow Jul 6, 2017

: should be =

Contributor Author

selmanj Jul 7, 2017

Fixed

google/provider.go Outdated

@@ @@ -297,3 +298,39 @@ func convertStringArr(ifaceArr []interface{}) []string { @@
               	}
               	return arr
               }
+              // flattenComputeMetadata transforms a list of MetadataItems (as returned via the GCP client) into a simple map from key

Contributor

danawillow Jul 6, 2017

This probably belongs either in the resource file itself or in metadata.go (generally flatten/expand functions are in the same file as the rest of the resource though)

Contributor Author

selmanj Jul 7, 2017

I moved it to metadata.go as it works with both project metadata and instance metadata

google/provider.go Outdated

+              		if val.Value == nil {
+              			continue
+              		}
+              		if v2, ok := m[val.Key]; ok {

Contributor

danawillow Jul 6, 2017

nit: should v2 just be v?

Contributor Author

selmanj Jul 7, 2017

Probably, although val should have a better name too. Will fix names.

google/provider.go Outdated

+              	idx := 0
+              	for key, value := range m {
+              		metadata[idx] = &compute.MetadataItems{Key: key, Value: &value}

Contributor

danawillow Jul 6, 2017

idx++? (you can also do metadata = append(metadata, &compute.MetadataItems{Key: key, Value: &value}). I think since the backing array won't need to be resized, it should have the same performance but I'm not 100% sure.)

Contributor Author

selmanj Jul 7, 2017

I am undone

will fix

selmanj force-pushed the add_fine_grained_project_metadata branch from 407a312 to f7b6c8f Compare

July 10, 2017 21:47

danawillow mentioned this pull request

Terraform removes non-managed project metadata #39

Closed

selmanj force-pushed the add_fine_grained_project_metadata branch from f7b6c8f to 6ecfd95 Compare

July 13, 2017 16:21

danawillow approved these changes

View reviewed changes

Contributor

danawillow commented Jul 17, 2017

make testacc TEST=./google TESTARGS='-run=TestAccComputeProjectMetadataItem'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeProjectMetadataItem -timeout 120m
=== RUN   TestAccComputeProjectMetadataItem_importBasic
--- PASS: TestAccComputeProjectMetadataItem_importBasic (38.07s)
=== RUN   TestAccComputeProjectMetadataItem_basic
--- PASS: TestAccComputeProjectMetadataItem_basic (22.98s)
=== RUN   TestAccComputeProjectMetadataItem_basicWithEmptyVal
--- PASS: TestAccComputeProjectMetadataItem_basicWithEmptyVal (22.58s)
=== RUN   TestAccComputeProjectMetadataItem_basicUpdate
--- PASS: TestAccComputeProjectMetadataItem_basicUpdate (36.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	120.403s

👍

selmanj added 3 commits

July 17, 2017 16:26


          Add support for google_compute_project_metadata_item

7988ec2

This allows terraform users to manage single key/value items within the
project metadata map, rather than the entire map itself.


          Update CHANGELOG.md

17db37f


          Add details about import

0b50bd5

selmanj force-pushed the add_fine_grained_project_metadata branch from 6ecfd95 to 0b50bd5 Compare

July 17, 2017 23:28

selmanj merged commit 236c0f5 into hashicorp:master

selmanj deleted the add_fine_grained_project_metadata branch

July 17, 2017 23:28

danawillow added a commit to danawillow/terraform-provider-google that referenced this pull request


          Revert "Add support for google_compute_project_metadata_item (hashico…

bade201

…rp#176)"

This reverts commit 236c0f5.

danawillow added a commit that referenced this pull request


          Revert metadata item changes (#225)

884158d

* Revert "Add additional fingerprint error to check for when updating metadata (#221)"

This reverts commit 4c8f62e.

* Revert "Fix bug where range variable is improperly dereferenced (#217)"

This reverts commit 8f75c1c.

* Revert "Add support for google_compute_project_metadata_item (#176)"

This reverts commit 236c0f5.

selmanj restored the add_fine_grained_project_metadata branch

September 17, 2017 19:49

negz pushed a commit to negz/terraform-provider-google that referenced this pull request


          Add support for google_compute_project_metadata_item (hashicorp#176)

c7a59d1

* Add support for google_compute_project_metadata_item

This allows terraform users to manage single key/value items within the
project metadata map, rather than the entire map itself.

* Update CHANGELOG.md

* Add details about import

negz pushed a commit to negz/terraform-provider-google that referenced this pull request


          Revert metadata item changes (hashicorp#225)

37b71c6

* Revert "Add additional fingerprint error to check for when updating metadata (hashicorp#221)"

This reverts commit 4c8f62e.

* Revert "Fix bug where range variable is improperly dereferenced (hashicorp#217)"

This reverts commit 8f75c1c.

* Revert "Add support for google_compute_project_metadata_item (hashicorp#176)"

This reverts commit 236c0f5.

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request


          Add support for google_compute_project_metadata_item (hashicorp#176)

f1ca47c

* Add support for google_compute_project_metadata_item

This allows terraform users to manage single key/value items within the
project metadata map, rather than the entire map itself.

* Update CHANGELOG.md

* Add details about import

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request


          Revert metadata item changes (hashicorp#225)

5fc653d

* Revert "Add additional fingerprint error to check for when updating metadata (hashicorp#221)"

This reverts commit 4c8f62e.

* Revert "Fix bug where range variable is improperly dereferenced (hashicorp#217)"

This reverts commit 8f75c1c.

* Revert "Add support for google_compute_project_metadata_item (hashicorp#176)"

This reverts commit 236c0f5.

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request


          Merge pull request hashicorp#176 from modular-magician/codegen-pr-991

cc7e0b6

Accommodate the new iamcredentials service in tests

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request


          Merge pull request hashicorp#176 from modular-magician/codegen-pr-991

f9d0914

Accommodate the new iamcredentials service in tests

danawillow mentioned this pull request

google_compute_project_metadata_item overwrites existing key/value #5514

Closed

ghost commented Mar 31, 2020

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!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels