-
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
Add Appengine's firewall rules. #1081
Conversation
I see a few issues still here, but first, @rileykarson - how do I specify that a variable should use |
Generated tests don't support org-level resources yet unfortunately! I haven't dug too much into anything more complex than compute-style project-level resources. |
@rileykarson: Thanks! Understood! Would you mind if I added it in this PR, and if that's okay, would a simple implementation of magic variable names (e.g. |
For sure! I'd probably scope it just to |
523cc1d
to
5b00913
Compare
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 5d956cc) 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 a382b9f) 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 d4a55df) have been included in your existing downstream PRs. |
Let me know when this is rebased to include #1091 |
da37b12
to
e50b002
Compare
Just finished. :) |
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. |
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.
Thanks! Wanted to make sure that you weren't waiting for any action on my part.
Just a few more small comments, mostly around the import part now that that's settled.
// relying on the behavior that an unset field will return the empty | ||
// value for the appropriate type. So if the result is a number, we'll | ||
// set it as a number, but if it's a string, we'll set it as a string. | ||
val, _ := d.GetOk(fieldName) |
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.
Can't we use d.Get
?
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 tried that, but there's no .Get() on this type.
@@ -25,7 +26,37 @@ func parseImportId(idRegexes []string, d TerraformResourceData, config *Config) | |||
// Starting at index 1, the first match is the full string. | |||
for i := 1; i < len(fieldValues); i++ { | |||
fieldName := re.SubexpNames()[i] | |||
d.Set(fieldName, fieldValues[i]) | |||
// This part looks confusing. Because there is no way to know at |
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 this looks that confusing anymore- we should still have a short comment explaining that we're using d.Get
in an unintuitive way to sniff the type of the field, but otherwise what we're doing is fairly sensible, converting a string-type input to the proper output type.
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.
Shrunk comment. Still need to explain why it's not a try/catch pattern because that will help future code archaeologists avoid making mistakes while refactoring.
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit d13cb73) have been included in your existing downstream PRs. |
…tests without breaking OiCS.
Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
d015ac0
to
3cc5854
Compare
This adds the Magic Modules generated resource for App Engine Firewall Rules.
Fixes hashicorp/terraform-provider-google#2478.
[all]
[terraform]
[terraform-beta]
[ansible]
[inspec]