terraform-google-modules / terraform-google-project-factory

Creates an opinionated Google Cloud project by using Shared VPC, IAM, and Google Cloud APIs
https://registry.terraform.io/modules/terraform-google-modules/project-factory/google
Apache License 2.0
826 stars 535 forks source link

seed project script misses dependency of billing budget feature #486

Closed haf closed 3 years ago

haf commented 3 years ago
"billingbudgets.googleapis.com"

Should be enabled for the seed project, to avoid an error when provisioning the first project / host-project / service-project.

thiagonache commented 3 years ago

should we add it to the provider?

thiagonache commented 3 years ago

I'm starting to think that we should have a new resource called google_project_seed that enables all APIs required... so you can get rid of string comparisons to figure out the issue, thoughts?

morgante commented 3 years ago

I don't think we have a mechanism for doing so and actually think the error raised by Terraform is quite clear.

haf commented 3 years ago

It's not really about clarify, it's about the amount of work it takes to discover all these edge cases. When I use software I expect one of these two cases to hold:

  1. The software works according to the samples and docs. What is documented is enough to use the software. The software does not crash.
  2. The software works according to samples, but documents "prerequisites", to be done manually, and links to how that manual work is performed. Beyond what is documented, the software never crashes.

Generally, crashing is a horrible user experience and means the software is faulty.

The fact that I keep hitting these edge cases completely derailed our setup project last week, taking more than a single day. In fact, knowing about your hidden edge cases, makes this project useless, in hindsight: I should have used existing Terraform code I had. But that's the thing about hindsight; you never know if the software you intend to use is broken, like this is.

thiagonache commented 3 years ago

Hi @haf ,

After trying to figure how to accomplish what I thought, as stated by @morgante , there's no way to do that due to how Google's API works. Also, all the requirements are documented.

We cannot say the module is broken :)

My understanding is that crashing and return an error are very different things

haf commented 3 years ago

no way to do that ...

Here's how I've done it before; or are we talking about different things? Enabling a service is an idempotent op in any case, so even if you're duplicating code that enables them, you're better off for doing so, despite that meaning that you have multiple resources managing the same service's state.


variable "apis" {
  description = "What services are enabled"
  type        = set(string)
  default = [
    "cloudapis.googleapis.com",
    "cloudbilling.googleapis.com",
  ]
}
resource "google_project_service" "gcp_services" {
  project                    = var.project_id
  disable_dependent_services = false
  disable_on_destroy         = false

  for_each = var.apis
  service = each.key
}

My understanding is that crashing and return an error are very different things

Errors are only not a crash when the user hasn't followed the docs, IMO. If there's no way to have the software work from just following the docs, it's broken. But it's all about what DX you want in the end.

thiagonache commented 3 years ago

The problem is not as simple as that. To enable a service API programmatically you need some APIs already enabled. Or the list of API required changes over time. It is documented here. The only change it would require is to add the API in the helper and says it is only required when the budget module is being used. Or even accept an ENUM to say if it should check all APIs or basic... ALL or STANDARD maybe? I'm a new contributor but I've noticed we take user experience very seriously.

PR is welcome.

morgante commented 3 years ago

Hi @haf, I'm sorry you had a frustrating experience but ultimately these pre-reqs are intrinsic to the set up.

As per your point, we actually do explicitly document this requirement: https://github.com/terraform-google-modules/terraform-google-project-factory#apis

Is there a reason that doc isn't sufficient for you?

Your sample would work for activating the API on the project, but that's not the issue. The API needs to be active on the service account project (where your credentials come from) which is outside the module.

haf commented 3 years ago

You're right, it's documented; I went by the precondition script + samples being enough, as my frame of mind was that any such scripts + boostrap functionality would enable the API:s it needed. My mistake.

morgante commented 3 years ago

We definitely should add this to the preconditions script, so sorry for that issue.