hashicorp / terraform-plugin-codegen-spec

Terraform Provider Code Generation Specification and Go Bindings
Mozilla Public License 2.0
7 stars 4 forks source link

Add associated_external_type to all attributes and blocks #36

Closed bendbennett closed 1 year ago

bendbennett commented 1 year ago

Background

Currently, associated_external_type is only allowed in the schema on data source, provider and resource single nested types. For example:

    "datasource_single_nested_attribute": {
      "type": "object",
      "additionalProperties": false,
      "properties": {
        "name": {
          "type": "string"
        },
        "single_nested": {
          "type": "object",
          "additionalProperties": false,
          "properties": {
            "associated_external_type": {
              "$ref": "#/$defs/schema_associated_external_type"
            },
            "attributes": {
              "$ref": "#/$defs/datasource_attributes"
            },

Proposal

bflad commented 1 year ago

Agreed this should be supported on all attribute types -- folks may need to rename or otherwise support odd casing of struct field names.

bendbennett commented 1 year ago

folks may need to rename or otherwise support odd casing of struct field names.

I was wondering about this. Do you think we might need something like the following so that we can specify the name that will be used for the field when populating the associated_external_type.type? For instance:

"associated_external_type": {
  "imports": [
    {
      "path": "github.com/aws/aws-sdk-go/service/imagebuilder"
    }
  ],
  "type": "*imagebuilder.ImageTestsConfiguration",
  "mapping": {
    "timeout_minutes": {
      "name": "MinutesTimeout",
    }
  }
},
bflad commented 1 year ago

I think so, personally. When I wrote up my initial thoughts on the spec and framework code generation pieces of this, I mentioned:

If IR Schema does not include a “mapping” field... then the following assumptions will be made when “expanding” the model to the API object:

If IR Schema does not include a “mapping” field... then the following assumptions will be made when “flattening” the API “object” to the model:

Presumably we can smooth over the naming parts slightly in the framework code generation by assuming that we should by default perform snake case to title case naming conversions (or vice versa) because Go SDKs will always have exported field names without underscores while Terraform will/should be the opposite. However, I think these are important code-level details that need to be customizable in some manner for developers because the real world is messy -- whether its the concept of "mapping" data with the associated_external_type or something else.

bflad commented 1 year ago

We can also handle those details in a followup change.

bendbennett commented 1 year ago

Ok. Sounds good. I'll create a follow-up issue to consider "mapping" or something along those lines to guide how the "expand" and "flatten" process should happen within the generated "expand" and "flatten functions - Consider adding "mapping" to associated_external_type.

github-actions[bot] commented 3 months ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.