fix: support passing service project number to shared_vpc_access to be tf 0.13 compatible

[umairidris]

Fix #499

[morgante]

@umairidris Would it be possible to use implicitly dependency to solve this instead? I'd rather not be adding more module_depends_on resources now that Terraform 0.13 supports it natively.

[bharathkkb]

We output project_number as part of core, so if we take that as an input for shared_vpc_access wouldn't that get rid of the need for module_depends_on?

[umairidris]

@morgante implicit dependencies don't work as intended due to hashicorp/terraform#26857.

@bharathkkb yes that was one option, but we will need to add a required var for project number which will be a breaking change on shared_vpc_access. Is that ok?

[morgante]

Wouldn't an optional variable be sufficient?

[umairidris]

Yes, that would work too though we would have two different paths (one with data source and one without). I can do that for now and we can remove the data source in a future major version.

[umairidris]

Or not...

Error: Invalid count argument

  on ../../modules/shared_vpc_access/main.tf line 18, in data "google_project" "service_project":
  18:   count      = var.service_project_number == null ? 1 : 0

The "count" value depends on resource attributes that cannot be determined
until apply, so Terraform cannot predict how many instances will be created.
To work around this, use the -target argument to first apply only the
resources that the count depends on.

[bharathkkb]

I think it's okay to have a breaking change with the upcoming v10.0 for shared_vpc_access. It is mostly used as a helper for shared_vpc module but I will defer to @morgante

[morgante]

This submodule is actually used directly frequently as well, so I'd definitely like to avoid a breaking change. More generally, I'd like to avoid forcing customers to think about project number at all. It's a confusing implementation detail of GCP and redundant with project ID.

How about we make a lookup_project_number boolean variable as well (default true)?

[umairidris]

If a v10 release is coming soon I would prefer to remove the data source entirely. It seems Terraform is pushing for data and provider blocks to be in the user's control more and more. In the meantime I have added the extra var.

[morgante]

I'm strongly against any change which forces users to provide their project number. This module is scattered all over and, frankly, is simply shifting the burden of confusion to the user's config and reducing the value of the module.

I also think this error is one we're unlikely to encounter outside project factory, so I'd rather take on a bit more complexity here in exchange for easier interfaces outside the module.

[umairidris]

That's a fair point, chances of this happening outside of PF are lower.

[bharathkkb]

LGTM
/cc @morgante

[umairidris]

@morgante does this look good to you?