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

terraform: Terraform module for GCP #2553

Merged
merged 6 commits into from
Nov 10, 2023
Merged

terraform: Terraform module for GCP #2553

merged 6 commits into from
Nov 10, 2023

Conversation

elchead
Copy link
Contributor

@elchead elchead commented Nov 3, 2023

Context

This PR builds on top of #2503 to add the module for GCP.

Proposed change(s)

  • add GCP module

Additional info

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from 6bf3ff4 to 8407c58 Compare November 3, 2023 15:23
@elchead elchead added the feature This introduces new functionality label Nov 3, 2023
@elchead elchead added this to the v2.13.0 milestone Nov 3, 2023
@elchead elchead force-pushed the feat/cli/gcp-tf-module branch 11 times, most recently from 3e1e07f to 2297685 Compare November 7, 2023 09:21
@elchead elchead requested a review from msanft November 7, 2023 10:20
@elchead elchead changed the title terraform: Terraform GCP module terraform: Terraform module for GCP Nov 7, 2023
@msanft
Copy link
Contributor

msanft commented Nov 7, 2023

Let's put this on hold until #2503 is merged.

@msanft msanft added the hold This cannot be merged right now label Nov 7, 2023
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

LGTM from a high level. Will give this a more thorough review once the AWS module is merged.

terraform/constellation-cluster/main.tf Show resolved Hide resolved
terraform/gcp-constellation/main.tf Outdated Show resolved Hide resolved
terraform/gcp-constellation/main.tf Outdated Show resolved Hide resolved
@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from d2c0d68 to e65e0e5 Compare November 7, 2023 18:38
@msanft
Copy link
Contributor

msanft commented Nov 8, 2023

Regarding release pipeline changes, @malt3 should review that.

Base automatically changed from feat/cli/terraform-module to main November 8, 2023 18:10
@elchead elchead requested a review from malt3 November 8, 2023 18:11
@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from e65e0e5 to 4773afc Compare November 8, 2023 18:21
Copy link

netlify bot commented Nov 8, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit ba9fce4
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/654d31dd4d9b5900081b3c8d

@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from 4773afc to 790a2d4 Compare November 8, 2023 18:29
@elchead elchead removed the hold This cannot be merged right now label Nov 8, 2023
@elchead elchead requested a review from msanft November 8, 2023 18:32
@elchead
Copy link
Contributor Author

elchead commented Nov 8, 2023

Rebased and incorporated feedback. Would you test it also locally on Mac to see if it works as expected for you @msanft?

@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from 790a2d4 to f2b89a4 Compare November 8, 2023 18:39
@msanft
Copy link
Contributor

msanft commented Nov 9, 2023

Rebased and incorporated feedback. Would you test it also locally on Mac to see if it works as expected for you @msanft?

I don't have a MacOS machine set up for development right now unfortunately

@elchead elchead force-pushed the feat/cli/gcp-tf-module branch 2 times, most recently from 45882ca to a819b86 Compare November 9, 2023 11:11
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

TF specific changes LGTM except the comment. Didn't look at release pipeline or Go upgrade. Just ping me once ready for final review

.github/workflows/e2e-test-tf-module.yml Show resolved Hide resolved
Copy link
Contributor

@malt3 malt3 left a comment

Choose a reason for hiding this comment

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

Only reviewed the changes made to the pipeline. They look good to me.
Apologies for the delay in reviewing from my side.

@msanft
Copy link
Contributor

msanft commented Nov 9, 2023

One thing that is broken here, but that will be addressed in #2566:

We cannot default to a latest version when downloading the CLI. This will break for users that have an outdated Terraform module, say of v2.12.0, and try to apply it at a point where v2.13.0 is already released. latest will then point to v2.13.0. If the v2.13.0 CLI is incompatible with the v2.12.0 module, this won't work.

We should rather let the user set the CLI version through a variable in the constellation-cluster module and set it during packaging of the module. I will take care of that in #2566.

@elchead
Copy link
Contributor Author

elchead commented Nov 9, 2023

One thing that is broken here, but that will be addressed in #2566:

We cannot default to a latest version when downloading the CLI. This will break for users that have an outdated Terraform module, say of v2.12.0, and try to apply it at a point where v2.13.0 is already released. latest will then point to v2.13.0. If the v2.13.0 CLI is incompatible with the v2.12.0 module, this won't work.

We should rather let the user set the CLI version through a variable in the constellation-cluster module and set it during packaging of the module. I will take care of that in #2566.

This is already fixed, Thomas and I decided to inject the version during the release process:

Update the `version` inside `terraform/constellation-cluster/install-constellation.sh` to the new release.

We didn't want to expose it as variable to the user, since the user should not need to care about the CLI version and which one is compatible with which TF module.

@elchead elchead force-pushed the feat/cli/gcp-tf-module branch from a819b86 to ba9fce4 Compare November 9, 2023 19:24
@elchead elchead merged commit 22d82a5 into main Nov 10, 2023
@elchead elchead deleted the feat/cli/gcp-tf-module branch November 10, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This introduces new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants