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

Create project as host project (shared_vpc_host) #464

Closed
wants to merge 15 commits into from
Closed

Create project as host project (shared_vpc_host) #464

wants to merge 15 commits into from

Conversation

thiagonache
Copy link
Contributor

Hi @morgante @bharathkkb ,

After a lot of discussions, I'm opening a new PR with a clean branch. I hope all is correct now.

… done via VPC anymore. Also, renames variable shared_vpc_enabled to enable_shared_vpc_service_project
…_service_project. Also, adds the new variable enable_shared_vpc_host_project
…vpc_host). Also, updates the logic to set enable_shared_vpc_service_project and enable_shared_vpc_host_project
@morgante
Copy link
Contributor

morgante commented Oct 1, 2020

The error is here:

terraform_validate ./test/fixtures/full 

Error: Unsupported argument

  on .terraform/modules/vpc/main.tf line 58, in resource "google_compute_subnetwork" "subnetwork":
  58:   enable_flow_logs         = lookup(var.subnets[count.index], "subnet_flow_logs", "false")

An argument named "enable_flow_logs" is not expected here.

@thiagonache
Copy link
Contributor Author

thiagonache commented Oct 1, 2020 via email

@thiagonache
Copy link
Contributor Author

I don’t think it is related to my changes... do you?

On 1 Oct 2020, at 16:56, Morgante Pell @.***> wrote:  The error is here: terraform_validate ./test/fixtures/full Error: Unsupported argument on .terraform/modules/vpc/main.tf line 58, in resource "google_compute_subnetwork" "subnetwork": 58: enable_flow_logs = lookup(var.subnets[count.index], "subnet_flow_logs", "false") An argument named "enable_flow_logs" is not expected here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

What I've found is that enable_flow_logs was deprecated, we should use log_config instead. Trying to match the same version that is working for me.

@thiagonache
Copy link
Contributor Author

thiagonache commented Oct 1, 2020

@morgante for some odd reason the google provider version pulled by my terraform execution end up with a different version than in the CI environment. I've deleted the terraform cache and confirmed that it pulled out the version 3.40 again, so I've created the required version config for the provider in fixtures/full/versions.tf and it worked.
The last thing I've just done is to revert the commit to pin the module version that was my initial suspicion. I'm confident that it will work this time.

@thiagonache
Copy link
Contributor Author

yay!

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

When creating the host project you should not need a shared_vpc input variable with the name.

examples/shared_vpc/main.tf Outdated Show resolved Hide resolved
modules/core_project_factory/variables.tf Outdated Show resolved Hide resolved
modules/gsuite_enabled/variables.tf Outdated Show resolved Hide resolved
modules/shared_vpc/variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@thiagonache thiagonache requested a review from morgante October 2, 2020 03:10
@thiagonache
Copy link
Contributor Author

thiagonache commented Oct 2, 2020

@morgante okay. you're right, but I think we should rename shared_vpc_enabled on the next major release as we will do with shared_vpc, do you agree?
Just finishing up to run tests locally and I will push changes

Co-authored-by: Morgante Pell <morgante.pell@morgante.net>
@morgante
Copy link
Contributor

morgante commented Oct 2, 2020

that's the problem with the logic and mentioned. We agreed on that...
the only way to determine the variable is if shared_vpc is set

No it's not. There is no reason to set shared_vpc on the host project—it already is the host project. Just ignore it completely. It's very confusing to pass a name to the shared_vpc argument on the host project.

@thiagonache
Copy link
Contributor Author

that's the problem with the logic and mentioned. We agreed on that...
the only way to determine the variable is if shared_vpc is set

No it's not. There is no reason to set shared_vpc on the host project—it already is the host project. Just ignore it completely. It's very confusing to pass a name to the shared_vpc argument on the host project.

yeah... I've removed this comment. Please, see my last comment

@morgante
Copy link
Contributor

morgante commented Oct 2, 2020

@morgante okay. you're right, but I think we should rename shared_vpc_enabled on the next major release as we will do with shared_vpc, do you agree?

Yep.

@thiagonache
Copy link
Contributor Author

Please, see related PR.

@thiagonache thiagonache closed this Oct 2, 2020
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.

2 participants