-
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
adding new resource to allow iam bindings on GCE instances #3551
adding new resource to allow iam bindings on GCE instances #3551
Conversation
Hey @reechar-goog! MM is the primary home for the codebase, and this repo is basically a read replica. We mirror most contributions back to MM to lower the barrier to contributing; we have a script that should mirror this easily once review is done, so keeping it here is fine. @slevenick do you want to take a crack at review on this? Feel free to assign back to me if you don't have the cycles. |
@rileykarson got, it, thanks! I have one more feature that I plan on contributing, but its currently beta only, so I'll target that one towards MM. |
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.
Looks good to me other than the spacing issues in the terraform config in the test file
Thanks for the addition!
I'll upstream this after changes to make it generate both here and in the beta provider
fixing whitespace/tabbing issue
@slevenick spacing issue addressed, Thanks for the review, let me know if you need anything else! |
@reechar-goog Looks like we will need documentation for this resource as well. Could you add documentation similar to https://github.com/terraform-providers/terraform-provider-google/blob/master/website/docs/r/google_service_account_iam.html.markdown As well as a link to that documentation that will appear on the website? |
@slevenick got it, will do, I'll work on that this afternoon and have something in a few hours |
@slevenick ready for review |
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.
Mostly spacing/docs comments. Looks good after that!
@slevenick hey sorry for all the spacing issues, finally got all my local tools set up and I have a better understanding of how to use them. I also reduced the tests so that they're more targeted towards what needs to be tested. Got the feedback from @rileykarson on another PR I'm working on. |
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.
Looks good pending minor docs updates. When those are fixed I'll upstream this to MM
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 👉 [email protected]. Thanks! |
This PR address issue: #3343
Should this also/instead be a PR against magic modules?