SetResource does not handle String Pointers properly in `GenerateCrawler` #1078

AaronME · 2021-11-24T21:56:33Z

Describe the bug
aws-controllers-k8s/code-generator#232 introduced the ability to support different goTypes for input/output shapes with the same name. This code is not working for all fields on all resources.

The Crawler Resource on the Glue API exposes a field "Schedule."

In CreateCrawlerInput, "Schedule" is a string.
In GetCrawlerOutput's Crawler, "Schedule" is a type Schedule.

When running this through the latest code-generator, we get the following on the GenerateCrawler method:

	if resp.Crawler.Schedule != nil {
		var f13 string
		cr.Spec.ForProvider.Schedule = f13
	} else {
		cr.Spec.ForProvider.Schedule = nil
	}

Because Schedule is a *string and not a string, this code does not compile.

Steps to reproduce
See generated code in https://github.com/crossplane/provider-aws/blob/2212c0919ed36e8d10f7829ef939d69322766b7e/pkg/controller/glue/crawler/zz_conversions.go#L92

Expected outcome
I would expect the generator to initialize this value as nil for a pointer:

	if resp.Crawler.Schedule != nil {
		cr.Spec.ForProvider.Schedule = resp.Crawler.Schedule
	} else {
		cr.Spec.ForProvider.Schedule = nil
	}

Environment

Kubernetes version N/A
Using EKS (yes/no), if so version? N/A
AWS service targeted (S3, RDS, etc.) glue

The text was updated successfully, but these errors were encountered:

vijtrip2 · 2021-11-24T22:48:08Z

Hi @AaronME , thanks for pointing this out.

This issue of different go types for same named field was recently fixed. Please take a look at this fix

You'll need to update generator.yaml to provide code-generator the instructions on how to handle this field. See the above mentioned fix's commit message for reference.

AaronME · 2021-11-24T23:50:43Z

@vijtrip2 Thank you for the fast reply.

I had already tried running generation with the following:

resources:
  Crawler:
    fields:
      Schedule:
        set:
          - on: Create
            from: ScheduleExpression
          - on: Update
            from: ScheduleExpression
          - on: ReadOne
            from: ScheduleExpression
          - on: ReadMany
            from: ScheduleExpression

This did not change any generated code.

I have two theories about this:

This is a known issue with nested fields, since the Schedule field needs to be set from ScheduleExpression, which is an attribute of Schedule.
allow different Go types for input/output shapes code-generator#232 does not affect setResourceReadMany (which is where GenerateCrawler is being generated).

jaypipes · 2021-11-29T15:15:54Z

2. allow different Go types for input/output shapes code-generator#232 does not affect setResourceReadMany (which is where GenerateCrawler is being generated).

Hi @AaronME! It's this second one. I will try to push a fix for this ASAP.

jaypipes · 2021-11-29T17:45:31Z

allow different Go types for input/output shapes code-generator#232 does not affect setResourceReadMany (which is where GenerateCrawler is being generated).

Hi @AaronME! It's this second one. I will try to push a fix for this ASAP.

Actually, it turns out that Glue's GetCrawler (Singular form/ReadOne) is used and not the list form of GetCrawlers (ReadMany) so this must be some other problem. Looking further into it...

jaypipes · 2021-11-29T18:05:12Z

Yeah, it's the lack of nested struct support (list item 1 above). I'm working on a fix now.

AaronME · 2021-11-29T19:08:10Z

@jaypipes I appreciate you taking a look at it.

In attempting to implement the nested field path support for different source and target field Go types, I realized I needed a support package that made working with field paths easier, including taking a field path and finding a ShapeRef within an Output shape that matched the field path. This patch introduces a new `pkg/fieldpath` package with a simple `Path` struct containing a variety of utility methods, as shown in this example Go code snippet: ```go import ( "fmt" awssdkmodel "github.com/aws/aws-sdk-go/private/model/api" "github.com/aws-controllers-k8s/code-generator/pkg/fieldpath" "github.com/aws-controllers-k8s/code-generator/pkg/sdk" ) ... p := fieldpath.FromString("Crawler.Schedule.ScheduleExpression") // Will print "true" fmt.Println(p.HasPrefix("Crawler.Schedule")) // Will print "false" fmt.Println(p.HasPrefix("crawler.Schedule")) // Will print "true" fmt.Println(p.HasPrefixFold("crawler.Schedule")) // Will print "ScheduleExpression" fmt.Println(p.Back()) // Will print "Crawler" fmt.Println(p.Front()) // Find the shape within the GetCrawlerResponse shape that contains the // field referred to by the path... helper := sdk.NewHelper("/path/to/apis", genCfg) api := helper.API("glue") crawlerOutputShape := api.Shapes["GetCrawlerResponse"] schedExpressionShape := p.ShapeRef(crawlerOutputShape) // Will print "ScheduleExpression" fmt.Println(schedExpressionShape.ShapeName) ``` Note that we support list and map type field paths using a single-dot notation since we are only looking at field types and not **values** within the list or map. Signed-off-by: Jay Pipes <jaypipes@gmail.com> Related: aws-controllers-k8s/community#1078 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

muvaf · 2021-12-07T21:57:54Z

We have similar situation in lambda.Function where Layers field is []*string in CreateFunctionInput but []*Layer in FunctionConfiguration which is the struct returned by CreateFunctionWithContext.

a-hilaly · 2021-12-08T13:46:19Z

We have similar situation in lambda.Function where Layers field is []*string in CreateFunctionInput but []*Layer in FunctionConfiguration which is the struct returned by CreateFunctionWithContext.

I am aware of this issue. For now our lambda controller is ignoring the Layers field. Will /cc you one the PR fixing that once it's open

jaypipes · 2022-01-05T15:29:21Z

@AaronME Sorry for the long delay in addressing this. I have pushed up aws-controllers-k8s/code-generator#255 to address the issue.

ack-bot · 2022-04-05T17:40:59Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-bot · 2022-05-05T17:41:26Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

ack-bot · 2022-06-04T23:13:58Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

ack-bot · 2022-06-04T23:14:01Z

@ack-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

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.

AaronME added the kind/bug Categorizes issue or PR as related to a bug. label Nov 24, 2021

jaypipes added the area/code-generation Issues or PRs as related to controllers or docs code generation label Nov 29, 2021

jaypipes self-assigned this Nov 29, 2021

jaypipes added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 29, 2021

jaypipes added this to To do in ACK Core Team Nov 29, 2021

jaypipes mentioned this issue Dec 1, 2021

introduce pkg/fieldpath for processing field paths aws-controllers-k8s/code-generator#248

Merged

AaronME mentioned this issue Dec 2, 2021

Upgrading to latest code-generator commit crossplane-contrib/provider-aws#920

Merged

2 tasks

muvaf mentioned this issue Dec 7, 2021

Ignore fields in glue.Crawler and lambda.Function that block code-generator bump crossplane-contrib/provider-aws#992

Merged

2 tasks

ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 5, 2022

RedbackThomson added this to To do in ACK Core Team Apr 8, 2022

ack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 5, 2022

ack-bot closed this as completed Jun 4, 2022

wotolom mentioned this issue Jul 6, 2022

enhance existing Glue resources crossplane-contrib/provider-aws#1380

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SetResource does not handle String Pointers properly in `GenerateCrawler` #1078

SetResource does not handle String Pointers properly in `GenerateCrawler` #1078

AaronME commented Nov 24, 2021

vijtrip2 commented Nov 24, 2021

AaronME commented Nov 24, 2021 •

edited

Loading

jaypipes commented Nov 29, 2021

jaypipes commented Nov 29, 2021

jaypipes commented Nov 29, 2021

AaronME commented Nov 29, 2021

muvaf commented Dec 7, 2021

a-hilaly commented Dec 8, 2021

jaypipes commented Jan 5, 2022

ack-bot commented Apr 5, 2022

ack-bot commented May 5, 2022

ack-bot commented Jun 4, 2022

ack-bot commented Jun 4, 2022

SetResource does not handle String Pointers properly in GenerateCrawler #1078

SetResource does not handle String Pointers properly in GenerateCrawler #1078

Comments

AaronME commented Nov 24, 2021

vijtrip2 commented Nov 24, 2021

AaronME commented Nov 24, 2021 • edited Loading

jaypipes commented Nov 29, 2021

jaypipes commented Nov 29, 2021

jaypipes commented Nov 29, 2021

AaronME commented Nov 29, 2021

muvaf commented Dec 7, 2021

a-hilaly commented Dec 8, 2021

jaypipes commented Jan 5, 2022

ack-bot commented Apr 5, 2022

ack-bot commented May 5, 2022

ack-bot commented Jun 4, 2022

ack-bot commented Jun 4, 2022

SetResource does not handle String Pointers properly in `GenerateCrawler` #1078

SetResource does not handle String Pointers properly in `GenerateCrawler` #1078

AaronME commented Nov 24, 2021 •

edited

Loading