-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Initial autogeneration of Spanner instance in Terraform. #1266
Conversation
@ the automatic unpacking of names, I'd like to get that done / I've been thinking about that as part of https://github.com/GoogleCloudPlatform/magic-modules/issues/937. I don't think I have time to work on it this quarter though. |
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit b5dd67f) have been included in your existing downstream PRs. |
078d2c8
to
b5fe5d9
Compare
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 151377c) have been included in your existing downstream PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented mostly on the TPG downstream
@@ -0,0 +1,3 @@ | |||
// Spanner instances are, ironically, eventually consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the name of this post_create isn't spanner specific, and this is reusable without this line, let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
obj["name"] = fmt.Sprintf("projects/%s/instances/%s", project, obj["name"]) | ||
newObj := make(map[string]interface{}) | ||
newObj["instance"] = obj | ||
updateMask := make([]string, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need new code for this updatemask instead of the existing MM support / existing custom code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - the resource update function takes instance
- the change object - rather than taking the fields to change at the top level. See https://cloud.google.com/spanner/docs/reference/rest/v1/projects.instances/patch vs https://cloud.google.com/compute/docs/reference/rest/v1/urlMaps/patch.
Woah, my machine did something insane there, some kind of YAML reflow situation gone horribly, horribly wrong. I'll fix that. |
7d14ec7
to
e366604
Compare
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 2c2bbf7) have been included in your existing downstream PRs. |
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 19e0bb1) have been included in your existing downstream PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented downstream & here with a couple assertions about name
, otherwise LGTM
exports: | ||
- name | ||
transport: !ruby/object:Api::Resource::Transport | ||
decoder: decode_response | ||
collection_url_response: !ruby/object:Api::Resource::ResponseList | ||
items: 'instances' | ||
<%= indent(compile_file({}, 'templates/global_async.yaml.erb'), 4) %> | ||
properties: | ||
- !ruby/object:Api::Type::String | ||
name: 'name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be marked input
, and required
? From the website docs:
Required. A unique identifier for the instance, which cannot be changed after the instance is created. Values are of the form projects//instances/[a-z][-a-z0-9]*[a-z0-9]. The final segment of the name must be between 6 and 30 characters in length.
For Terraform, we can preserve the optional name w/ overrides of course!
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 6afed74) have been included in your existing downstream PRs. |
Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
f26e24e
to
25e59c0
Compare
This one is a little odd - it's got an autogenerate-if-not-set feature for
name
. I know we don't usually do that, but since it already exists, I threw some custom code in there to handle it.There's also a few minor issues requiring an encoder, update encoder, and decoder. The encoder and update encoder are necessary because the put and patch methods take nested objects, somewhat unusually. The decoder is necessary because project and name are both packed into name... I'm thinking of adding a feature to Magic Modules allowing the automatic unpacking of names into other fields. Has that already been talked about?
[all]
[terraform]
[terraform-beta]
[ansible]
Description / doc changes related to spanner instances in Terraform.
[inspec]