-
Notifications
You must be signed in to change notification settings - Fork 131
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 GCP to terraform deployment examples #433
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.
+@adam-singer
+@MarcusSorealheis
Reviewable status: 0 of 37 files reviewed, all discussions resolved (waiting on @adam-singer and @MarcusSorealheis)
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.
Reviewable status: 0 of 37 files reviewed, 1 unresolved discussion (waiting on @adam-singer and @MarcusSorealheis)
deployment-examples/terraform/GCP/README.md
line 3 at r1 (raw file):
# TODO Documentation coming soon.
I don't want to hold this PR up on documentation, since we have other work to do, so going to put the documentation in the backlog.
c015c06
to
ffd659d
Compare
I'm reviewing this one tonight but it is beefy and must be tested so will take me a few hours. |
can_ip_forward = false | ||
|
||
service_account { | ||
# Google recommends custom service accounts that have cloud-platform scope and permissions granted via IAM Roles. |
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.
it's important. No human identity should be tied to permissions in a script.
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 we should hold landing this up. There are some minor things I would do differently, but if this works, or almost works, I'm confident the community will benefit from it being in the project.
6c054be
to
add3434
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.
Reviewable status: 0 of 37 files reviewed, 2 unresolved discussions (waiting on @adam-singer and @MarcusSorealheis)
deployment-examples/terraform/GCP/module/instance_cas.tf
line 73 at r2 (raw file):
Previously, MarcusSorealheis (Marcus Eagan) wrote…
it's important. No human identity should be tied to permissions in a script.
Done.
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 would like to just get it in. I have been testing extensively and every time I find a minor issue another issue raises. I won't land it before I'm confident it fully works as-is.
Reviewable status: 0 of 37 files reviewed, 2 unresolved discussions (waiting on @adam-singer and @MarcusSorealheis)
add3434
to
7513073
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.
lgtm. Post landing would be great for us all to independently test on a gcp sandbox setup if possible. I do wonder if that will be a bit of churn with the cert init.
Reviewable status: 0 of 37 files reviewed, 2 unresolved discussions (waiting on @MarcusSorealheis)
7513073
to
90f744c
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.
Dismissed @MarcusSorealheis from a discussion.
Reviewable status: 0 of 37 files reviewed, all discussions resolved (waiting on @adam-singer and @MarcusSorealheis)
Adds Google Cloud deploymnet example to the "deployment-examples" terraform directory.
90f744c
to
3b00d73
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.
Reviewable status: 0 of 37 files reviewed, all discussions resolved (waiting on @MarcusSorealheis)
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.
Reviewed 29 of 37 files at r1, 2 of 2 files at r2, 3 of 5 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @allada)
Adds Google Cloud deployment example to the "deployment-examples" terraform directory.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)