-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add support for Google RuntimeConfig #315
Conversation
Manages a RuntimeConfig resource in Google Cloud. For more information, see the | ||
[official documentation](https://cloud.google.com/deployment-manager/runtime-configurator/), | ||
or the | ||
[JSON API](hhttps://cloud.google.com/deployment-manager/runtime-configurator/reference/rest/). |
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.
remove extra "h"
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
|
||
resource "google_runtimeconfig_variable" "environment" { | ||
parent = "${google_runtimeconfig_config.my-runtime-config.name}" | ||
name = "environment" |
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 it would be a more realistic example if you were storing a value in a hierarchy instead.
...
name = "prod-variables/hostname"
text = "example.com"
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.
Fair enough, using a '/' in example now
Manages a RuntimeConfig variable in Google Cloud. For more information, see the | ||
[official documentation](https://cloud.google.com/deployment-manager/runtime-configurator/), | ||
or the | ||
[JSON API](hhttps://cloud.google.com/deployment-manager/runtime-configurator/reference/rest/). |
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.
same here, remove extra "h"
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.
Oops, done
var runtimeConfigFullName *regexp.Regexp | ||
|
||
func init() { | ||
runtimeConfigFullName = regexp.MustCompile("^projects/([^/]+)/configs/(.+)$") |
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 don't think you need this init block, you can simply do:
var runtimeConfigFullName *regexp.Regexp = regexp.MustCompile("^projects/([^/]+)/configs/(.+)$")
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.
Didn't know you could do this; for some reason I thought only primitives could be initialized in this manner (I guess this works because it's a var rather than a constant?)
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.
correct, a const can't be set at runtime
Name: fullName, | ||
} | ||
|
||
if val, ok := d.GetOk("description"); ok { |
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 you can simply this by removing the if-check and keeping only:
runtimeConfig.Description = d.Get("description").(string)
If description is not set, d.Get("description").(string)
will return the default value ""
(empty string).
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 didn't do this because I feel like GetOk
reads better, although I agree they are equivalent.
runtimeConfig.Description = val.(string) | ||
} | ||
|
||
_, err = config.clientRuntimeconfig.Projects.Configs.Create("projects/"+project, &runtimeConfig).Do() |
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.
No waiting on the operation to complete? Is this synchronous?
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.
It is!
Name: fullName, | ||
} | ||
// Update the description (currently its the only thing that could have changed) | ||
if v, ok := d.GetOk("description"); ok { |
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.
use d.HasChange
. I know right now this is the only field that may change but if we add more fields, it will be easier to extend.
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.
Spoke in person; this update API is confusing in that it's more like 'overwrite'. Added comments to help explain this.
Name: fullName, | ||
} | ||
// Update the description (currently its the only thing that could have changed) | ||
if v, ok := d.GetOk("description"); ok { |
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 believe this logic is wrong. If you set the description, apply, then unset it, and re-apply, then the description won't be erased. You only change the description if it is set but unsetting it is also something the user might want to do. using hasChange instead like suggested in my previous comment will solve this.
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.
Same as above; update is like overwrite. Added comments.
|
||
"value": { | ||
Type: schema.TypeString, | ||
Optional: true, |
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.
add a ConflictsWith with the "text" field since only one or the other can be set.
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.
Didn't know this one! Added!
This allows users to create/manage Google RuntimeConfig resources and variables. More information here: https://cloud.google.com/deployment-manager/runtime-configurator/ Closes hashicorp#236
Also adds better comments around how update works
01514c6
to
b4219c6
Compare
* Vendor runtimeconfig * Add support for RuntimeConfig config and variable resources This allows users to create/manage Google RuntimeConfig resources and variables. More information here: https://cloud.google.com/deployment-manager/runtime-configurator/ Closes hashicorp#236 * Remove typo * Use top-level declaration rather than init() * Cleanup testing-related code by using ConflictsWith Also adds better comments around how update works
* Vendor runtimeconfig * Add support for RuntimeConfig config and variable resources This allows users to create/manage Google RuntimeConfig resources and variables. More information here: https://cloud.google.com/deployment-manager/runtime-configurator/ Closes hashicorp#236 * Remove typo * Use top-level declaration rather than init() * Cleanup testing-related code by using ConflictsWith Also adds better comments around how update works
* Vendor runtimeconfig * Add support for RuntimeConfig config and variable resources This allows users to create/manage Google RuntimeConfig resources and variables. More information here: https://cloud.google.com/deployment-manager/runtime-configurator/ Closes hashicorp#236 * Remove typo * Use top-level declaration rather than init() * Cleanup testing-related code by using ConflictsWith Also adds better comments around how update works
<!-- This change is generated by MagicModules. --> /cc @rileykarson
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! |
This allows users to create/manage Google RuntimeConfig resources and
variables. More information here:
https://cloud.google.com/deployment-manager/runtime-configurator/
Closes #236