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.

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

Merged
merged 1 commit into from
Dec 2, 2023
Merged

Add GCP to terraform deployment examples #433

merged 1 commit into from
Dec 2, 2023

Conversation

allada
Copy link
Member

@allada allada commented Dec 1, 2023

Adds Google Cloud deployment example to the "deployment-examples" terraform directory.


This change is Reviewable

Copy link
Member Author

@allada allada left a 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)

Copy link
Member Author

@allada allada left a 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.

@allada allada force-pushed the add-terraform-gcp branch from c015c06 to ffd659d Compare December 1, 2023 03:47
@MarcusSorealheis
Copy link
Collaborator

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.
Copy link
Collaborator

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.

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a 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.

@allada allada force-pushed the add-terraform-gcp branch 2 times, most recently from 6c054be to add3434 Compare December 1, 2023 15:44
Copy link
Member Author

@allada allada left a 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.

Copy link
Member Author

@allada allada left a 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)

@allada allada force-pushed the add-terraform-gcp branch from add3434 to 7513073 Compare December 1, 2023 16:34
Copy link
Contributor

@adam-singer adam-singer left a 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)

@allada allada force-pushed the add-terraform-gcp branch from 7513073 to 90f744c Compare December 1, 2023 23:15
Copy link
Member Author

@allada allada left a 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.
@allada allada force-pushed the add-terraform-gcp branch from 90f744c to 3b00d73 Compare December 1, 2023 23:44
Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 37 files reviewed, all discussions resolved (waiting on @MarcusSorealheis)

Copy link
Member Author

@allada allada left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @allada)

@allada allada merged commit 4661a36 into main Dec 2, 2023
14 checks passed
@allada allada deleted the add-terraform-gcp branch December 2, 2023 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants