Support nested Ignore.FieldPaths #119

chlunde · 2021-07-05T22:06:11Z

The current implementation of IgnoreFields did not support nested
shapes. For Ignore.FieldPaths: Foo.Bar.Baz, it would ignore the whole of
Bar.

With this change, it supports multiple levels of nested fields.

A practical example for this is ses CreateTemplateInput.Template.TemplateName.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot · 2021-07-05T22:06:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chlunde
To complete the pull request process, please assign redbackthomson after the PR has been reviewed.
You can assign the PR to them by writing /assign @redbackthomson in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-bot · 2021-07-05T22:06:19Z

Hi @chlunde. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chlunde · 2021-07-12T09:35:43Z

pkg/model/model.go

 		return
 	}
 	for sdkShapeID, shape := range m.SDKAPI.API.Shapes {
+	ignoreFields:


I don't like this, I can extract this function later... 🌴

@chlunde checkout this very similar function :) https://github.com/aws-controllers-k8s/code-generator/pull/78/files#diff-f5c09cb6b638098a0e513f710e901c9f7381165e4e2c49f2b3881613d2db518dR324 - the first implementation was using a goto statement as well #78 (comment)

@chlunde ping! :) Did you have a chance to check out the code and comment from @a-hilaly above?

back from vacation today! I'll try to fix it by tomorrow

The current implementation of IgnoreFields did not support nested shapes. For Ignore.FieldPaths: Foo.Bar.Baz, it would ignore the whole of Bar. With this change, it supports multiple levels of nested fields. A practical example for this is ses CreateTemplateInput.Template.TemplateName. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly · 2021-08-03T10:03:06Z

/ok-to-test

a-hilaly

LGTM overall 👍 I left two comments below.

a-hilaly · 2021-08-03T11:28:17Z

pkg/model/model_eks_test.go

+		"Version",
+		"ScalingConfig.DesiredSize",


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

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?

a-hilaly · 2021-08-03T11:29:36Z

pkg/model/model.go

 	"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"


nit: goimports can help with separating internal from external imports

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

code-generator/pkg/model/field.go

Lines 19 to 22 in 2f11b23

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.

muvaf · 2021-08-03T16:54:42Z

pkg/testdata/models/apis/eks/0000-00-00/generator.yaml

+    # Test nested field path of multiple levels
+    - Nodegroup.Version
+    - CreateNodegroupInput.Version
+    - CreateNodegroupInput.ScalingConfig.DesiredSize


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?

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 :)

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2021

ack-bot requested review from RedbackThomson and a-hilaly July 5, 2021 22:06

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2021

chlunde mentioned this pull request Jul 5, 2021

Implement AWS SES Template crossplane-contrib/provider-aws#755

Closed

2 tasks

chlunde marked this pull request as ready for review July 9, 2021 22:13

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021

chlunde commented Jul 12, 2021

View reviewed changes

ack-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2021

a-hilaly reviewed Aug 3, 2021

View reviewed changes

muvaf reviewed Aug 3, 2021

View reviewed changes

chlunde closed this Aug 3, 2021

	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"

Support nested Ignore.FieldPaths #119

Support nested Ignore.FieldPaths #119

Uh oh!

Conversation

chlunde commented Jul 5, 2021

Uh oh!

ack-bot commented Jul 5, 2021

Uh oh!

ack-bot commented Jul 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Aug 3, 2021

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

a-hilaly Jul 12, 2021 •

edited

Loading