Add product resource support#447
Conversation
demeyerthom
left a comment
demeyerthom
left a comment
There was a problem hiding this comment.
Big PR :). I left a few comments on things i would like to see changed.
I am curious at your usecase here though. I would never recommend putting an actual product into terraform state, as the data is normally much to ephermal to be considered configuration (same with stuff like customers, prices etc).
Mind you, I am not opposed to merging this pr to add this to the provider, but I am trying to understand why it is necessary
| Optional: true, | ||
| }, | ||
| }, | ||
| Blocks: map[string]schema.Block{ |
There was a problem hiding this comment.
I think it would be better to follow the terraform suggestion here to not use blocks but instead nested attributes: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/blocks. This is not something we have been doing consistently in the current code base, but is something I want to change to at some point (in a newer version of the provider). Would it be possible to make that change?
There was a problem hiding this comment.
Initially, I tried this one, but I encountered an error. I assumed that it may be due to the current "semi-migrated" state, but I could be mistaken.
2023/11/15 15:46:05 error retrieving schema for *proto5server.Server:
Attribute:
Summary: Error converting resource schema
Detail: The schema for the resource "commercetools_product" couldn't be converted into a usable type. This is always a problem with the provider. Please report the following to the provider developer:
AttributeName("example_attribute"): protocol version 5 cannot have Attributes set
| ) | ||
|
|
||
| // Product represents the main schema data. | ||
| type Product struct { |
There was a problem hiding this comment.
I am not 100% sure about this data model. The actual implementation for a product has the masterData field, which contains both the current and staged versions of the product. I am uncertain if we want to mirror this data model in the provider directly, or if we want to reduce the complexity a bit by making the assumption that a product that is managed in terraform should always be in a published state (so we can basically ignore staged, as it will always be the same as published). What is your usecase here?
There was a problem hiding this comment.
I have implemented the approach you described where the stage and current values are always the same.
Personally, I do not see the need to manage these values separately in the Terraform template.
| func marshalAttributeValue(o platform.Attribute) string { | ||
| val, err := json.Marshal(o.Value) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Can we return this error instead of panic-ing? It allows terraform to deal with the failing resource a bit more elegantly (this might stop the whole application midway processing, which might cause side-effects when other processes are running in parallel)
There was a problem hiding this comment.
I will fix this.
|
|
||
| type Attribute struct { | ||
| Name types.String `tfsdk:"name"` | ||
| Value types.String `tfsdk:"value"` |
There was a problem hiding this comment.
We might also want to model this value as a custom tfsdk value instead: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/types/custom. That way we can still use the power of defined types within the sdk, without hiding the complexity as marshalled json strings
There was a problem hiding this comment.
I will have a look at this one as well.
The use case is to create discount templates that need to be populated along with the configuration. |
|
Any updates on this? |
|
Hi @ghandic no currently no movement on this. If @bulat-khusnimardanov feels like picking it up he can, and I would be happy to review. But this has no priority for us at the moment, so keeping it in draft |
3 participants
This merge request adds support for creating and managing Product resources.
Mock server extension is a prerequisite: labd/commercetools-node-mock#122
NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS