labd / terraform-provider-commercetools

Terraform provider for commercetools
https://registry.terraform.io/providers/labd/commercetools/latest/docs
Mozilla Public License 2.0
64 stars 67 forks source link

Ignore values in enums types in product type, named attributes #486

Closed piran-cabiri closed 1 month ago

piran-cabiri commented 5 months ago

We have a product type with some enums in them. The product data is fed by an external system, and so we add enum values to the product type when a new value appears.

Currently I define my product type like this:

resource "commercetools_product_type" "main" {
  key  = "main"
  name = "main"

  # attr 0 - Brand - See lifecycle block below
  attribute {
    name = "brand"
    label = {
      "en-GB" = "Brand"
    }
    required = false
    type {
      name = "enum"
      # additional values are added during product load
      value {
        key   = "apple"
        label = "Apple"
      }
      value {
        key   = "bose"
        label = "Bose"
      }
    }
    constraint = "SameForAll"
    searchable = false
  }

  # attr 1 - Collections - See lifecycle block below
  attribute {
    name = "collections"
    label = {
      "en-GB" = "Collections"
    }
    required = false
    type {
      name = "set"
      element_type {
        name = "enum"
        value {
          key   = "100 and Under"
          label = "100 and Under"
        }
      }
    }
    constraint = "SameForAll"
    searchable = false
  }

  # ... other attributes

  lifecycle {
    ignore_changes = [
      attribute.0.type, # brand
      attribute.1.type, # collections
    ]
  }
}

Then if I add enum types via the API, with:

POST https://api.eu-central-1.aws.commercetools.com/my-proj/product-types/key=main
{
  "version" : 58,
  "actions" : [ 
    {
        "action" : "addPlainEnumValue",
        "attributeName" : "brand",
        "value": {
            "key": "coke",
            "label": "Coke"
        }
    },
    {
        "action" : "addPlainEnumValue",
        "attributeName" : "collections",
        "value": {
            "key": "200 and Under",
            "label": "200 and Under"
        }
    }
  ]
}

Now if I reapply the Terraform, the ignore_changes ignores the fact that I've added or deleted enum values. Brilliant!

But there are a few niggles:

  1. When I first create the attributes, I have to leave out thelifecycle segment. Somehow the attributes seems to get created correctly, and getting the product type it looks good, but the API call above gives the complaint that their ltext and not enums. Commenting out the lifecycle fixes this.

  2. It'd be much nicer if I could name rather than number attributes, see below. Currently that fails to work correctly. I also tried attribute["brand"].type but it starts ignoring a lot more than just the type then. Using numbers makes it all very sensitive to someone else putting attributes in the wrong place.

  lifecycle {
    ignore_changes = [
      attribute.brand.type, # doesn't work
      attribute.collections.type, #  doesn't work
    ]
  }
  1. It would be even better if it were just the values that were ignored (eg, attribute.brand.type.value), so I could see changes of type such as enum to/from set.

I'm using commercetools provider 1.14.1, terraform 1.8.1, and terragrunt v0.55.5

demeyerthom commented 5 months ago

Hi @piran-cabiri thanks for reaching out!

So if I can summarize your situation:

With regards to your additional points:

  1. I would have to check what is happening here, but this might have to do with the slice of attributes not always being in the same order?
  2. I fear 2 and 3 are not possible unless we do a full rewrite of how terraform stores these attributes: we currently just store them as an ordered list (the same way commercetools expects the data), so the only key terraform knows about is the index of the attribute. FYI in go a slice is also not guaranteed to be in the same order, so the number is not guaranteed to be the same either here, which adds another wrinkle to your strategy above.

My main question here would be why use an enum at all? It sounds like (from the standpoint of Commercetools) this value is a simple text field. Is there a reason you want the extra check on allowed input here? Would it not be easier to just sync updates from this external system as text?

piran-cabiri commented 5 months ago

Hi @demeyerthom. Your summary is accurate.

W.r.t. to your main question: The principal reason is that these values are used to drive promotions. With a large number of values, it's easy for merchandisers make mistakes in setting up promotion rules. Storing as enums or a set of enums means that the promotions UI in Merchant Centre provides a searchable&clickable set of values and prevents mistakes: e.g., someone typing "addidas" instead of "adidas" (or in deed "Adidas"). We're less concerned about how a product itself appears in the Merchant Centre, as product data is managed in another system. I have been through Commercetools support to confirm they are happy with their end of the solution in terms of the number of enums and the frequency of update; they only worry was the effect on indexing for projections, but we use an external search tool.

W.r.t. point 2, based on your description, if you moved the order to be a new property of the attribute in the terraform stored state, you could reconstruct the ordering, and perhaps allow reference by either name or number? I do not know enough of the TF internals to answer that. (Perhaps https://github.com/labd/terraform-provider-commercetools/blob/main/commercetools/resource_product_type.go#L273 and elsewhere?)

demeyerthom commented 4 months ago

Hi @piran-cabiri

Ok, the usecase is clear to me!

I delved a bit further in the code to see what it was doing.

Firstly we are comparing the names of attributes to get the right change sets, so the ordering should not be an issue as I implied earlier. We check the old and new list for existing attribute names, and process the changes accordingly.

I tried out creating the resources with the lifecycle.ignore_changes hook, but that seems to work fine for me. I can create it no problem, and I can change the enums after that, and it will not attempt updating anything after either.

An additional fix here to make it even more specific might be the following:

  lifecycle {
    ignore_changes = [
      attribute.0.type.0.value, # brand
      attribute.1.type.0.element_type.0.value, # collections
    ]
  }

This should let you track everything except for the values of the fields

With regards to the last one: changing this would change the API of the configuration, as the objects we specify are directly related to the way terraform stores the data. Changing these in any structural way basically means that the configuration would not be backwards compatible (unless we start hacking around it with migrations, something I don't want to do)