VSChina / magic-modules

Magic Modules: Automagically generate Google Cloud Platform support for OSS
Apache License 2.0
1 stars 4 forks source link

Expect short names in flatten functions #63

Open houkms opened 4 years ago

houkms commented 4 years ago

Auto-Gened:

if natGatewayPropertiesFormat := resp.NatGatewayPropertiesFormat; natGatewayPropertiesFormat != nil {

Expected:

if props:= resp.NatGatewayPropertiesFormat; props != nil {   
houkms commented 4 years ago

I expect it's only the root property in Read function need such modifications.

magodo commented 4 years ago

I expect it's only the root property in Read function need such modifications.

What if there is also nested object inside another nested object? E.g. we have following nested objects:

type Foo struct {
    *Bar
}

type Bar struct {
    *Baz
}

type Baz struct {
    Name string
}

func main() {
    f := &Foo{
        &Bar{
            &Baz{
                Name: "foobar",
            },
        },
    }

    if prop := f.Bar; prop != nil {
                // `prop` below has different identity
        if prop := prop.Baz; prop != nil {
            fmt.Println(prop.Name)
        }
    }
}

Take note at the comment above, where prop has different identity in one line. Does this kind of style introduce other issue?

houkms commented 4 years ago

I suppose this case will not appear. This issue is essentially a naming convention of Hashicorp, specifically, when using client.response to set the go SDK filed, they require:

MM now always uses go_field_name as the tmp variable name to accept the response values, which is what Hashicorp complains about.

For your specific example. it should be in this way (because Baz is a nestedOject, we need a flatten function).

if prop := f.Bar; prop != nil {
     // `prop` below has different identity
    if err := d.set("Baz", flattenBaz(prop.Baz)){
        return fmt.Error(" ... ")
    }
}

If Baz is not a nestedObject, we can do it in this way

if prop := f.Bar; prop != nil {
    // `prop` below has different identity
    if baz := prop.Baz; baz != nil {
        return fmt.Error(" ... ")
    }
}
magodo commented 4 years ago

After digging further, I found that part of naming is controled by file: nested_object_shcema_assign.erb, as below:

<%
    ...
    temp_var = sdk_type.go_variable_name || sdk_type.go_field_name.camelcase(:lower) || input
-%>
...

And this part is only used for this purpose. From the code above we can see the var name is resolved via prioprity: go_variable_name > go_field_name > input (input is a passed in argument, e.g., 'resp')

So I suppose for this issue, we can just modify the read::response section in terraform.yaml, adding go_variable_name: properties to /properties field.

magodo commented 4 years ago

Update last reply

If we want the embeded part of unmarshalling(i.e. sdk -> schema) to follow same naming convention on tmp var, we need to apply the same naming convention used in nested_object_schema_assign.erb in other places.