terraform-google-modules / terraform-google-slo

Creates SLOs on Google Cloud from custom Stackdriver metrics capability to export SLOs to Google Cloud services and other systems
https://registry.terraform.io/modules/terraform-google-modules/slo/google
Apache License 2.0
63 stars 29 forks source link

BUGFIX - Bad default value for dataset_default_table_expiration_ms #16

Closed bkamin29 closed 4 years ago

bkamin29 commented 4 years ago

We have a bug with the default value of the variable dataset_default_table_expiration_ms: a number or null value is required, not a string like "".

bkamin29 commented 4 years ago

@ocervell, il y avait un bug sur ma précédente PR .. Est-ce que tu sais pourquoi l'un des tests est en fail ?

ocervell commented 4 years ago

@benesis002 seems like defaulting to null breaks the module if it is not passed: Terraform is expecting a number and null doesn't work.

Any idea to solve the issue ?

We could pass -1 and make a check to set it to null if -1 is passed, what do you think ?

bkamin29 commented 4 years ago

@benesis002 seems like defaulting to null breaks the module if it is not passed: Terraform is expecting a number and null doesn't work.

Any idea to solve the issue ?

We could pass -1 and make a check to set it to null if -1 is passed, what do you think ?

Hello @ocervell,

null work in my context, i use this terraform module declaration :

module "slo_pipeline" {
  source                              = "github.com/terraform-google-modules/terraform-google-slo//modules/slo-pipeline?ref=b1829b3dd42349ff116ca43528a13574ee0d6eb5"
  project_id                          = var.gcp_project.id
  region                              = var.region
  //slo_generator_version               = local.slo_generator_version
  slo_generator_version               = "1.0.0a1"
  storage_bucket_location             = var.region
  storage_bucket_storage_class        = "REGIONAL"
  dataset_default_table_expiration_ms = null
  exporters = [
    {
      class      = "Stackdriver"
      project_id = var.gcp_project.id
    },
    {
      class                      = "Bigquery"
      project_id                 = var.gcp_project.id
      dataset_id                 = "slo"
      table_id                   = "reports"
      location                   = "EU"
      delete_contents_on_destroy = false
    }
  ]
}

I will do some tests today to understrand your problem ;)

bkamin29 commented 4 years ago

@ocervell I have try to use 0 and -1 value instead of null value for default_table_expiration_ms attribute in resource "google_bigquery_dataset" "main". It's don't work :

Error: "default_table_expiration_ms" cannot be shorter than 3600000 milliseconds (one hour)

  on .terraform/modules/slo_pipeline.slo_pipeline/modules/slo-pipeline/main.tf line 47, in resource "google_bigquery_dataset" "main":
  47: resource "google_bigquery_dataset" "main" {

It is why i have chose to use null

ocervell commented 4 years ago

Fixed in #19 - doing a default to -1 and converting to null when calling the resource ;)