Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for IAM on Spanner Instances. #1387

Merged
merged 6 commits into from
May 3, 2018
Merged

Add support for IAM on Spanner Instances. #1387

merged 6 commits into from
May 3, 2018

Conversation

paddycarver
Copy link
Contributor

Support managing IAM policies on Spanner instances.

Tests:

make testacc TEST=./google TESTARGS='-run=TestAccSpannerInstanceIam'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccSpannerInstanceIam -timeout 120m
=== RUN   TestAccSpannerInstanceIamBinding
=== PAUSE TestAccSpannerInstanceIamBinding
=== RUN   TestAccSpannerInstanceIamMember
=== PAUSE TestAccSpannerInstanceIamMember
=== RUN   TestAccSpannerInstanceIamPolicy
=== PAUSE TestAccSpannerInstanceIamPolicy
=== CONT  TestAccSpannerInstanceIamBinding
=== CONT  TestAccSpannerInstanceIamPolicy
=== CONT  TestAccSpannerInstanceIamMember
--- PASS: TestAccSpannerInstanceIamPolicy (14.49s)
--- PASS: TestAccSpannerInstanceIamMember (22.42s)
--- PASS: TestAccSpannerInstanceIamBinding (29.29s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 29.295s

@paddycarver
Add support for IAM on Spanner Instances.
e686ea5 
Support managing IAM policies on Spanner instances.
@paddycarver paddycarver requested review from nat-henderson, rosbo and a team April 26, 2018 14:47
mbfrahry
mbfrahry previously approved these changes Apr 26, 2018
View reviewed changes
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paddycarver
Copy link
Contributor Author

Just noticed I neglected to update the docs for this, I'll push a docs update momentarily.

@paddycarver
Copy link
Contributor Author

This has docs now. Still look good, @mbfrahry?

nat-henderson
nat-henderson reviewed May 2, 2018
View reviewed changes
website/docs/r/spanner_instance_iam.html.markdown

Three different resources help you manage your IAM policy for a Spanner instance. Each of these resources serves a different use case:

* `google_spanner_instance_iam_policy`: Authoritative. Sets the IAM policy for the instance and replaces any existing policy already attached.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a warning note here - we've gotten a user complaint or two that the docs don't strongly enough warn about the consequences of using foo_iam_policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'll do this for the database IAM docs I'm writing right now, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a warning. Look good?

@paddycarver paddycarver dismissed mbfrahry’s stale review May 2, 2018 18:42

Added docs after review.

@paddycarver
Don't reimplement policy translation.
5ad4414 
We already have this in spaner_database_iam, no need to reimplement it
here.
nat-henderson
nat-henderson approved these changes May 3, 2018
View reviewed changes
Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay - warning looks good to me.

@paddycarver paddycarver merged commit afcd482 into master May 3, 2018
@paultyng paultyng deleted the paddy_spanner_instance_iam branch October 29, 2018 19:28
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@paddycarver
Merge pull request hashicorp#1387 from terraform-providers/paddy_span…
d86bbb2 
…ner_instance_iam

Add support for IAM on Spanner Instances.
@ghost
Copy link

ghost commented Nov 16, 2018

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!

@ghost ghost locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nat-henderson nat-henderson nat-henderson approved these changes

@mbfrahry mbfrahry mbfrahry left review comments

@rosbo rosbo Awaiting requested review from rosbo

Assignees
No one assigned
Labels
enhancement new-resource
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paddycarver @nat-henderson @mbfrahry