Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 37 additions & 4 deletions pkg/model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/aws-controllers-k8s/code-generator/pkg/generate/templateset"
"github.com/aws-controllers-k8s/code-generator/pkg/names"
"github.com/aws-controllers-k8s/code-generator/pkg/util"
awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
Copy link
Member

Choose a reason for hiding this comment

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

nit: goimports can help with separating internal from external imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think goimports places it the same place, it was placed there by gopls. I tried deleting the line and by running goimports -w ./pkg/model/model.go I get the same result.

The sibling file

ackgenconfig "github.com/aws-controllers-k8s/code-generator/pkg/generate/config"
"github.com/aws-controllers-k8s/code-generator/pkg/names"
"github.com/aws-controllers-k8s/code-generator/pkg/util"
awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
is also the same. But you're right, I think I've seen a convention where stdlib packages have one group, same-module has another group and all others have a third group.

)

var (
Expand Down Expand Up @@ -657,6 +658,28 @@ func (m *Model) GetEnumDefs() ([]*EnumDef, error) {
return edefs, nil
}

// getShapeMember recursively traverses the requested fieldpath. It
// does not support the .. syntax for lists. If the field cannot be
// resolved, nil is returned.
func getShapeMember(shape *awssdkmodel.Shape, fieldPath string) *awssdkmodel.Shape {
if fieldPath == "" {
return shape
}

fields := strings.Split(fieldPath, ".")

sh := shape
for _, fn := range fields {
shapeRef, _ := sh.MemberRefs[fn]
if shapeRef == nil {
return nil
}
sh = shapeRef.Shape
}

return sh
}

// ApplyShapeIgnoreRules removes the ignored shapes and fields from the API object
// so that they are not considered in any of the calculations of code generator.
func (m *Model) ApplyShapeIgnoreRules() {
Expand All @@ -665,12 +688,22 @@ func (m *Model) ApplyShapeIgnoreRules() {
}
for sdkShapeID, shape := range m.SDKAPI.API.Shapes {
for _, fieldpath := range m.cfg.Ignore.FieldPaths {
sn := strings.Split(fieldpath, ".")[0]
fn := strings.Split(fieldpath, ".")[1]
if shape.ShapeName != sn {
fields := strings.Split(fieldpath, ".")

shapeName := fields[0]
nestedFieldPath := strings.Join(fields[1:len(fields)-1], ".")
fieldName := fields[len(fields)-1]

if shape.ShapeName != shapeName {
continue
}
delete(shape.MemberRefs, fn)

sh := getShapeMember(shape, nestedFieldPath)
if sh == nil {
continue
}

delete(sh.MemberRefs, fieldName)
}
for _, sn := range m.cfg.Ignore.ShapeNames {
if shape.ShapeName == sn {
Expand Down
48 changes: 48 additions & 0 deletions pkg/model/model_eks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package model_test

import (
"testing"

"github.com/stretchr/testify/require"

"github.com/aws-controllers-k8s/code-generator/pkg/testutil"
)

func TestEKS_IgnoreFieldPaths(t *testing.T) {
require := require.New(t)

g := testutil.NewModelForService(t, "eks")

crds, err := g.GetCRDs()
require.Nil(err)

crd := getCRDByName("Nodegroup", crds)
require.NotNil(crd)

// fields we have ignored via generator.yaml
ignoredFieldPaths := []string{
"Version",
"ScalingConfig.DesiredSize",
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

Should'nt these be more Nodegroup.Version and CreateNodegroupInput.ScalingConfig.DesiredSize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already under getCRDByName("Nodegroup", ..), so logically it is now Nodegroup.ScalingConfig.DesiredSize.

But yeah, if we could rewrite it to a unit test then maybe it would look like that, or maybe we should test it through another code path than getCRDByName?

}
for _, ignoredFieldPath := range ignoredFieldPaths {
_, found := crd.Fields[ignoredFieldPath]
require.False(found, "field should be ignored: %v", ignoredFieldPath)
}

// Ensure we do not accidentally remove sibling fields
_, found := crd.Fields["ScalingConfig.MaxSize"]
require.True(found, "Sibling should be kept when ScalingConfig.DesiredSize is ignored")
}
7 changes: 7 additions & 0 deletions pkg/testdata/models/apis/eks/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
ignore:
field_paths:
# Test nested field path of multiple levels
- Nodegroup.Version
- CreateNodegroupInput.Version
- CreateNodegroupInput.ScalingConfig.DesiredSize
Copy link
Contributor

Choose a reason for hiding this comment

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

Quoting from the docstring:

// Set of field paths to ignore. The name here should be the original name of
// the field as it appears in AWS SDK objects. You can refer to a field by
// giving its "<shape_name>.<field_name>". For example, "CreateApiInput.Name".

Would it be possible to use ScalingConfig.DesiredSize (assuming ScalingConfig is the name of the type) to achieve what you'd like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that works 🤦 Thanks! :D

I was trying to help @ikarldasan on slack to get progress on crossplane-contrib/provider-aws#755 - but I didn't know how the generator config worked and dug into this hole.

So for that case, to ignore TemplateName, it would simply be:

ignore:
  field_paths:
    - Template.TemplateName

I though that was tested but apparently not :)


resources:
FargateProfile:
renames:
Expand Down