Support nested paths in field config specifically list type members #183

mbaijal · 2021-09-07T09:04:54Z

Issue #, if available:
aws-controllers-k8s/community#932

Description of changes:
getMemberByPath checks the shape type as it loops through members to ensure lists are handled correctly. Currently I have simply ignored the empty element created by the splitting of the string on the single dot instead of handling the double dot notation explicitly. Open to comments.
Also added a Unit Test for the same.

Testing

Generated code for ApplicationAutoscaling and ensured CreationTime field now works as expected. Linked changes in this PR.
Ensured that the SageMaker Controller can be generated using new changes with no diff as expected.

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

ack-bot · 2021-09-07T09:05:03Z

Hi @mbaijal. 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.

jaypipes · 2021-09-07T12:40:22Z

pkg/model/sdk_helper.go

 	for x, elem := range elements {
+		if elem == "" {
+			continue
+		}


Yeah, this is fine for now. I suppose we could also warn or panic if the .. notation was used to indicate a non-list/map field but I'm cool just ignoring for now.

Actually, this check is required since the path string in case of a list is currently split into 3 elements and we need to ignore the second one -

input Path - ScalableTargets..CreationTime
the elements array in line 254 - ["ScalableTargets", "", "CreationTime"]

and we need to make sure we don't process the second elem. Another option would be to directly parse for ".." and handle lists but that seemed unnecessary at the moment.

@jaypipes just making sure we are on the same page 👆🏻

Yep, we're all good.

jaypipes · 2021-09-07T12:40:53Z

pkg/model/sdk_helper.go

 	shape *awssdkmodel.Shape,
 	path string,
 ) (*awssdkmodel.ShapeRef, bool) {
 	elements := strings.Split(path, ".")


Might be worth calling out in the docstring that .. notation for list/map types is currently ignored.

sure, i will add the comment based on the discussion on the above comment.

Added a comment and TODO.

jaypipes · 2021-09-07T12:41:50Z

pkg/model/sdk_helper.go

+		elemType := shapeRef.Shape.Type
+		switch elemType {
+		case "list":
+			shape = shapeRef.Shape.MemberRef.Shape


Eventually we should support map type as well, which would be:

case "map": shape = shapeRef.Shape.ValueRef.Shape

You can do that in a followup PR, though, since this one is for lists only (and dealing with map types in the code generator is a little tougher anyway)

Sounds good, created an issue here - aws-controllers-k8s/community#936

RedbackThomson

Is this worthy of some simple unit tests in the sdk_helper_test.go file?

mbaijal · 2021-09-07T22:45:11Z

Is this worthy of some simple unit tests in the sdk_helper_test.go file?

Probably, let me give that a go.

a-hilaly

LGTM 👍

a-hilaly · 2021-09-08T09:18:14Z

/ok-to-test

a-hilaly · 2021-09-08T09:22:31Z

/lgtm

ack-bot · 2021-09-08T09:22:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, mbaijal, RedbackThomson

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,RedbackThomson,jaypipes]

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

…rces (#39) Issue #, if available: aws-controllers-k8s/community#932 Description of changes: Add creationTime to Status for ScalableTarget and ScalingPolicy resources based off of [code gen changes in this PR.](aws-controllers-k8s/code-generator#183) TODO: Add required check to the existing integration test. Testing Manually tested that the field is present in the Status of the `kubectl get` response. Kind tests pass locally By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot requested review from jaypipes and vijtrip2 September 7, 2021 09:04

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

mbaijal mentioned this pull request Sep 7, 2021

Add creationTime to Status for ScalableTarget and ScalingPolicy resources aws-controllers-k8s/applicationautoscaling-controller#39

Merged

jaypipes approved these changes Sep 7, 2021

View reviewed changes

ack-bot added the approved label Sep 7, 2021

RedbackThomson approved these changes Sep 7, 2021

View reviewed changes

mbaijal mentioned this pull request Sep 7, 2021

Support nested paths in field config specifically map type members aws-controllers-k8s/community#936

Closed

mbaijal force-pushed the fields-post-release branch from b6ba086 to 7ffdf76 Compare September 8, 2021 02:55

Support nested paths in field config

b32639c

mbaijal force-pushed the fields-post-release branch from 7ffdf76 to b32639c Compare September 8, 2021 05:58

a-hilaly approved these changes Sep 8, 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 Sep 8, 2021

ack-bot assigned a-hilaly Sep 8, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2021

ack-bot merged commit 9cebd89 into aws-controllers-k8s:main Sep 8, 2021

RedbackThomson mentioned this pull request Sep 8, 2021

Support nested paths in field config specifically list type members aws-controllers-k8s/community#932

Closed

Support nested paths in field config specifically list type members #183

Support nested paths in field config specifically list type members #183

Uh oh!

Conversation

mbaijal commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Sep 7, 2021

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!

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!

mbaijal Sep 7, 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!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

mbaijal commented Sep 7, 2021

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Sep 8, 2021

Uh oh!

a-hilaly commented Sep 8, 2021

Uh oh!

ack-bot commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mbaijal commented Sep 7, 2021 •

edited

Loading

mbaijal Sep 7, 2021 •

edited

Loading