hashicorp / terraform-google-vault

A Terraform Module for how to run Vault on Google Cloud using Terraform and Packer
Apache License 2.0
113 stars 75 forks source link

Fixes #39: Wrong GCS bucket name #40

Closed robmorgan closed 5 years ago

robmorgan commented 5 years ago

Relevant issue #39.

hashicorp-cla commented 5 years ago

CLA assistant check
All committers have signed the CLA.

Eoino commented 5 years ago

Possibly need to modify the following also, I'm guessing? Not sure if there are any others. https://github.com/hashicorp/terraform-google-vault/blob/07b2996798d3d5da177084421492f5ac73f946f4/modules/vault-cluster/main.tf#L299 https://github.com/hashicorp/terraform-google-vault/blob/07b2996798d3d5da177084421492f5ac73f946f4/modules/vault-cluster/main.tf#L312

robmorgan commented 5 years ago

@Eoino yep, definitely will. I guess we could probably use "${google_storage_bucket.vault_storage_backend.name}" here.

Etiene commented 5 years ago

For some reason I cant see the job on circle ci, I guess you need to sign the contributor agreement first... Or maybe you arent in the comitter list and it will only run once I merge it?

But I got a question, is this change backwards compatible? Wouldnt this destroy the bucket of someone already using this module?

robmorgan commented 5 years ago

Hi @Etiene, I've signed the CLA now. Yeah, I think the reason you can't see the build on CircleCI is because the branch is under my account - robmorgan. So you can either pull the branch across to the Hashicorp org somehow to trigger a build or merge it.

Yes its definitely not backwards compatible. It would destroy an existing bucket and attempt to create a new one. We could address this in the release notes or tell users to ensure ${var.gcs_bucket_name} has the same value as ${var.cluster_name}". That wouldn't trigger a change.

WDYT?

Etiene commented 5 years ago

What about doing something like this? name = "${var.gcs_bucket_name == "" ? var.cluster_name : var.gcs_bucket_name}"

Etiene commented 5 years ago

ping

robmorgan commented 5 years ago

Hey @Etiene, I've improved the code and docs a bit. Also, I've tested locally and I don't get a diff in the plan anymore between the master version and this branch. Therefore it should no longer be considered a breaking change.

Please merge + release when you're happy.