-
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
Terraform generate iam #2006
Terraform generate iam #2006
Conversation
1cda4ee
to
dd409a0
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in Ansible. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
9b538a9
to
7a0b0b9
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
This is great! I was working on something similar last week but didn't get half as far as this. Looking forward to have this in to start contributing some new IAM resources. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
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.
Looking good so far! I made a first pass, most of my comments are just nitpicks or have to do with primary_resource_name
. I focused mostly on the generated tests (🔥 🔥) so far, I'm waiting for the rename to hit the downstreams to dig more into the resource files when I can easily compare this to handwritten ones.
} | ||
|
||
resource "<%= terraform_name -%>_policy" "editor" { | ||
<%= object.name.underscore -%> = "<%= object.id_format.gsub('{{name}}', "{{#{object.name.underscore}}}")-%>" |
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.
Is the change to partial URI in pubsub intentional? Will specifying that work? https://github.com/terraform-providers/terraform-provider-google-beta/pull/914/files#diff-5bc4b7188e596ed679a95ddfd0c495c2R45
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.
Yeah, I believe changing to the partial URI makes the tests and examples more deterministic. Currently the pubsub topic IAM resources allow many different ways of specifying the topic that is the subject of IAM policies. For example topic = "topic-name"
uses the default project, but still works. Tests of these different ways of declaring it can be found here: https://github.com/terraform-providers/terraform-provider-google-beta/blob/master/google-beta/resource_pubsub_topic_iam_test.go and are still being used to test the generated version
It makes sense to me to be as specific as possible in test/example code rather than leaving the project unspecified. It also allows for the generation to be more generic
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'd guess that most users expect to use short names, and so I lean slightly more towards short names than partial URIs. That said, not strongly enough to block- the distinction doesn't end up mattering much, and it's easier for us if users specify the partial URI as well.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
1 similar comment
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
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.
LGTM! Just a few small things.
} | ||
|
||
resource "<%= terraform_name -%>_policy" "editor" { | ||
<%= object.name.underscore -%> = "<%= object.id_format.gsub('{{name}}', "{{#{object.name.underscore}}}")-%>" |
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'd guess that most users expect to use short names, and so I lean slightly more towards short names than partial URIs. That said, not strongly enough to block- the distinction doesn't end up mattering much, and it's easier for us if users specify the partial URI as well.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
c9cc2c6
to
82c2ecc
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
82c2ecc
to
c62ec3e
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Co-Authored-By: Riley Karson <[email protected]>
Co-Authored-By: Riley Karson <[email protected]>
c62ec3e
to
0b1d4f4
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
0b1d4f4
to
8c7c5ee
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
fixes: hashicorp/terraform-provider-google#2889
Add support for generating IAM resources to MM. This PR converts pubsub_topic_iam_* and adds new resources for source_repo_repository_iam_*
Release Note for Downstream PRs (will be copied)