-
Notifications
You must be signed in to change notification settings - Fork 263
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
Deprecate convenience fields in google_compute_snapshot #173
Conversation
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 only commented on the CustomizeDiff stuff, and only for one resource, so far, because I feel like updating it once will update it everywhere, right? And also, that's what I was specifically asked to give feedback on.
A few minor points, but overall, I think the logic covers the cases well, and makes sense to me?
I'll continue reviewing the rest, I just wanted to surface this early.
49bbb99
to
62c5fa0
Compare
Ah, nope - all of those were unintentional. Once the Magician is done they should be fixed. |
85100ad
to
788fd3e
Compare
788fd3e
to
a14dbad
Compare
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.
Assuming tests pass, the code looks reasonable to me.
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 comments as in hashicorp/terraform-provider-google#2572 (review) - once resolved, lgtm
a14dbad
to
d95b0db
Compare
<!-- This change is generated by MagicModules. --> /cc @rileykarson
<!-- This change is generated by MagicModules. --> /cc @rileykarson
/cc @rileykarson