Skip to content

allow different Go types for input/output shapes #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 11, 2021
Merged

allow different Go types for input/output shapes #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion pkg/generate/ack/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/aws-controllers-k8s/code-generator/pkg/generate/code"
ackgenconfig "github.com/aws-controllers-k8s/code-generator/pkg/generate/config"
"github.com/aws-controllers-k8s/code-generator/pkg/generate/templateset"
"github.com/aws-controllers-k8s/code-generator/pkg/model"
ackmodel "github.com/aws-controllers-k8s/code-generator/pkg/model"
awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
)
Expand Down Expand Up @@ -102,7 +103,18 @@ var (
return code.SetSDKForStruct(r.Config(), r, targetFieldName, targetVarName, targetShapeRef, sourceFieldPath, sourceVarName, indentLevel)
},
"GoCodeSetResourceForStruct": func(r *ackmodel.CRD, targetFieldName string, targetVarName string, targetShapeRef *awssdkmodel.ShapeRef, sourceVarName string, sourceShapeRef *awssdkmodel.ShapeRef, indentLevel int) string {
return code.SetResourceForStruct(r.Config(), r, targetFieldName, targetVarName, targetShapeRef, sourceVarName, sourceShapeRef, indentLevel)
f, ok := r.Fields[targetFieldName]
if !ok {
return ""
}
// We may have some special instructions for how to handle setting the
// field value...
setCfg := f.GetSetterConfig(model.OpTypeList)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why model.OpTypeList as operation?
I think adding a comment for the same will be useful as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why hard-code model.OpTypeList? From the PR description it looks like both Create and Update support setter configs as well.


if setCfg != nil && setCfg.Ignore {
return ""
}
return code.SetResourceForStruct(r.Config(), r, targetFieldName, targetVarName, targetShapeRef, setCfg, sourceVarName, sourceShapeRef, indentLevel)
},
"GoCodeCompare": func(r *ackmodel.CRD, deltaVarName string, sourceVarName string, targetVarName string, indentLevel int) string {
return code.CompareResource(r.Config(), r, deltaVarName, sourceVarName, targetVarName, indentLevel)
Expand Down
177 changes: 148 additions & 29 deletions pkg/generate/code/set_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,19 +181,6 @@ func SetResource(
continue
}

sourceMemberShapeRef := outputShape.MemberRefs[memberName]
if sourceMemberShapeRef.Shape == nil {
// Technically this should not happen, so let's bail here if it
// does...
msg := fmt.Sprintf(
"expected .Shape to not be nil for ShapeRef of memberName %s",
memberName,
)
panic(msg)
}

sourceMemberShape := sourceMemberShapeRef.Shape

// Determine whether the input shape's field is in the Spec or the
// Status struct and set the source variable appropriately.
var f *model.Field
Expand All @@ -219,6 +206,53 @@ func SetResource(
}

targetMemberShapeRef = f.ShapeRef

// We may have some special instructions for how to handle setting the
// field value...
setCfg := f.GetSetterConfig(opType)

if setCfg != nil && setCfg.Ignore {
continue
}

sourceMemberShapeRef := outputShape.MemberRefs[memberName]
if sourceMemberShapeRef.Shape == nil {
// We may have some instructions to specially handle this field by
// accessing a different Output shape member.
//
// As an example, let's say we're dealing with a resource called
// Bucket that has a "Name" field. In the Output shape of the
// ReadOne resource manager method, the field name is "Bucket",
// though. There may be a generator.yaml file that looks like this:
//
// resources:
// Bucket:
// fields:
// Name:
// set:
// - method: ReadOne
// from: Bucket
//
// And this would indicate to the code generator that the Spec.Name
// field should be set from the value of the Output shape's Bucket
// field.
var found bool
Comment on lines +212 to +239
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit cleanup suggestions:

setCfg := f.GetSetterConfig(opType)
sourceMemberShapeRef := outputShape.MemberRefs[memberName]

if sourceMemberShapeRef.Shape == nil && setCfg != nil {
    // We may have some instructions...
    if setCfg.Ignore != nil {
        continue
    }
    if setCfg.From != nil ...
}

if setCfg != nil && setCfg.From != nil {
sourceMemberShapeRef, found = outputShape.MemberRefs[*setCfg.From]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to supported nested field paths here, as well?

Copy link
Contributor

@brycahta brycahta Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I'm fairly certain I will need nested field paths support for DhcpOptions.

}
if !found || sourceMemberShapeRef.Shape == nil {
// Technically this should not happen, so let's bail here if it
// does...
msg := fmt.Sprintf(
"expected .Shape to not be nil for ShapeRef of memberName %s",
memberName,
)
panic(msg)
}
}

sourceMemberShape := sourceMemberShapeRef.Shape

// fieldVarName is the name of the variable that is used for temporary
// storage of complex member field values
//
Expand Down Expand Up @@ -270,6 +304,7 @@ func SetResource(
f.Names.Camel,
memberVarName,
targetMemberShapeRef,
setCfg,
sourceAdaptedVarName,
sourceMemberShapeRef,
indentLevel+1,
Expand All @@ -282,6 +317,22 @@ func SetResource(
)
}
default:
if setCfg != nil && setCfg.From != nil {
// We have some special instructions to pull the value from a
// different field or member...
sourceAdaptedVarName = sourceVarName + "." + *setCfg.From
var found bool
sourceMemberShapeRef, found = outputShape.MemberRefs[*setCfg.From]
if !found {
// This is likely a typo in the generator.yaml file...
msg := fmt.Sprintf(
"expected to find a member shape of %s called %s",
outputShape.ShapeName,
*setCfg.From,
)
panic(msg)
}
}
out += setResourceForScalar(
qualifiedTargetVar,
sourceAdaptedVarName,
Expand Down Expand Up @@ -502,6 +553,14 @@ func setResourceReadMany(
continue
}

// We may have some special instructions for how to handle setting the
// field value...
setCfg := f.GetSetterConfig(model.OpTypeList)

if setCfg != nil && setCfg.Ignore {
continue
}

targetMemberShapeRef = f.ShapeRef
out += fmt.Sprintf(
"%s\tif %s != nil {\n", indent, sourceAdaptedVarName,
Expand All @@ -526,6 +585,7 @@ func setResourceReadMany(
f.Names.Camel,
memberVarName,
targetMemberShapeRef,
setCfg,
sourceAdaptedVarName,
sourceMemberShapeRef,
indentLevel+2,
Expand Down Expand Up @@ -1100,6 +1160,8 @@ func setResourceForContainer(
targetVarName string,
// Shape Ref of the target struct field
targetShapeRef *awssdkmodel.ShapeRef,
// SetFieldConfig of the *target* field
targetSetCfg *ackgenconfig.SetFieldConfig,
// The struct or struct field that we access our source value from
sourceVarName string,
// ShapeRef of the source struct field
Expand All @@ -1113,6 +1175,7 @@ func setResourceForContainer(
targetFieldName,
targetVarName,
targetShapeRef,
targetSetCfg,
sourceVarName,
sourceShapeRef,
indentLevel,
Expand All @@ -1123,6 +1186,7 @@ func setResourceForContainer(
targetFieldName,
targetVarName,
targetShapeRef,
targetSetCfg,
sourceVarName,
sourceShapeRef,
indentLevel,
Comment on lines 1186 to 1192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow these are getting out of hand :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah at first glance looks like we might be able to refactor by passing Field type in directly instead of each of its fields or maybe extend Field type with these methods

Expand All @@ -1133,6 +1197,7 @@ func setResourceForContainer(
targetFieldName,
targetVarName,
targetShapeRef,
targetSetCfg,
sourceVarName,
sourceShapeRef,
indentLevel,
Expand All @@ -1158,6 +1223,8 @@ func SetResourceForStruct(
targetVarName string,
// Shape Ref of the target struct field
targetShapeRef *awssdkmodel.ShapeRef,
// SetFieldConfig of the *target* field
targetSetCfg *ackgenconfig.SetFieldConfig,
// The struct or struct field that we access our source value from
sourceVarName string,
// ShapeRef of the source struct field
Expand Down Expand Up @@ -1199,6 +1266,7 @@ func SetResourceForStruct(
cleanNames.Camel,
memberVarName,
targetMemberShapeRef,
nil,
sourceAdaptedVarName,
memberShapeRef,
indentLevel+1,
Expand Down Expand Up @@ -1236,6 +1304,8 @@ func setResourceForSlice(
targetVarName string,
// Shape Ref of the target slice field
targetShapeRef *awssdkmodel.ShapeRef,
// SetFieldConfig of the *target* field
targetSetCfg *ackgenconfig.SetFieldConfig,
// The struct or struct field that we access our source value from
sourceVarName string,
// ShapeRef of the source slice field
Expand All @@ -1244,6 +1314,7 @@ func setResourceForSlice(
) string {
out := ""
indent := strings.Repeat("\t", indentLevel)

sourceShape := sourceShapeRef.Shape
targetShape := targetShapeRef.Shape
iterVarName := fmt.Sprintf("%siter", targetVarName)
Expand All @@ -1257,26 +1328,71 @@ func setResourceForSlice(
targetShape.MemberRef.Shape,
indentLevel+1,
)
// f0elem0 = *f0iter0
// We may have some instructions to specially handle this field by
// accessing a different Output shape member.
//
// or
// As an example, let's say we're dealing with a resource called
// DBInstance that has a "DBSecurityGroups" field of type `[]*string`.
// In the Output shape of the ReadOne resource manager method, the
// field name is "DBSecurityGroups" of type
// `[]*DBSecurityGroupMembership`, though. This
// `DBSecurityGroupMembership` struct contains two fields,
// `DBSecurityGroupName` and `DBSecurityGroupStatus` and we wish to
// grab only the value of the `DBSecurityGroupName` fields and set the
// `Spec.DBSecurityGroups` field to the set of those field values.
//
// f0elem0.SetMyField(*f0iter0)
containerFieldName := ""
if sourceShape.MemberRef.Shape.Type == "structure" {
containerFieldName = targetFieldName
// There may be a generator.yaml file that looks like this:
//
// resources:
// DBInstance:
// fields:
// DBSecurityGroups:
// set:
// - method: ReadOne
// from: DBSecurityGroupName
Copy link
Contributor

@brycahta brycahta Nov 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Don't want to hold up review with this, but think interface can be improved.

Not a huge fan of DBSecurityGroups being used to reference both CRD field and Output Shape field and/or that the user needs background knowledge on this override (fields must be same name) in order to use it. This lack of clarity becomes more apparent when combining this config with output_wrapper_field_path for the same operation. Example is DhcpOptions:

operations:
  CreateDhcpOptions:
    output_wrapper_field_path: DhcpOptions

...

resources:
  DhcpOptions:
    fields:
      DhcpConfigurations:
        set:
          - on: Create
            from: Values

Since you are resolving a type mismatch issue here, what if the setconfig from reflected the path to the value of a given type -- not the exact field name in Output Shape? Something like:

	// resources:
	//   DBInstance:
	//     fields:
	//       DBSecurityGroups:
	//         set:
	//          - method: ReadOne
	//            from_type: DBSecurityGroupMembership.DBSecurityGroupName

//
// And this would indicate to the code generator that the
// Spec.DBSecurityGroups field should be set from the value of the
// Output shape's DBSecurityGroups..DBSecurityGroupName fields.
if targetSetCfg != nil && targetSetCfg.From != nil {
if sourceMemberShapeRef, found := sourceShape.MemberRef.Shape.MemberRefs[*targetSetCfg.From]; found {
out += setResourceForScalar(
elemVarName,
fmt.Sprintf("*%s.%s", iterVarName, *targetSetCfg.From),
sourceMemberShapeRef,
indentLevel+1,
)
} else {
// This is a bug in the code generation if this occurs...
msg := fmt.Sprintf(
"could not find targetSetCfg.From %s in struct member type.",
*targetSetCfg.From,
)
panic(msg)
}
} else {
// f0elem0 = *f0iter0
//
// or
//
// f0elem0.SetMyField(*f0iter0)
containerFieldName := ""
if sourceShape.MemberRef.Shape.Type == "structure" {
containerFieldName = targetFieldName
}
out += setResourceForContainer(
cfg, r,
containerFieldName,
elemVarName,
&targetShape.MemberRef,
targetSetCfg,
iterVarName,
&sourceShape.MemberRef,
indentLevel+1,
)
}
out += setResourceForContainer(
cfg, r,
containerFieldName,
elemVarName,
&targetShape.MemberRef,
iterVarName,
&sourceShape.MemberRef,
indentLevel+1,
)
addressOfVar := ""
switch sourceShape.MemberRef.Shape.Type {
switch targetShape.MemberRef.Shape.Type {
case "structure", "list", "map":
break
default:
Expand All @@ -1299,6 +1415,8 @@ func setResourceForMap(
targetVarName string,
// Shape Ref of the target map field
targetShapeRef *awssdkmodel.ShapeRef,
// SetFieldConfig of the *target* field
targetSetCfg *ackgenconfig.SetFieldConfig,
// The struct or struct field that we access our source value from
sourceVarName string,
// ShapeRef of the source map field
Expand Down Expand Up @@ -1332,6 +1450,7 @@ func setResourceForMap(
containerFieldName,
valVarName,
&targetShape.ValueRef,
nil,
valIterVarName,
&sourceShape.ValueRef,
indentLevel+1,
Expand Down
Loading