aws-controllers-k8s / community

AWS Controllers for Kubernetes (ACK) is a project enabling you to manage AWS services from Kubernetes
https://aws-controllers-k8s.github.io/community/
Apache License 2.0
2.41k stars 253 forks source link

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

Closed AaronME closed 2 years ago

AaronME commented 2 years ago

Describe the bug https://github.com/aws-controllers-k8s/code-generator/pull/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

vijtrip2 commented 2 years ago

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 commented 2 years ago

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

  1. This is a known issue with nested fields, since the Schedule field needs to be set from ScheduleExpression, which is an attribute of Schedule.
  2. https://github.com/aws-controllers-k8s/code-generator/pull/232 does not affect setResourceReadMany (which is where GenerateCrawler is being generated).
jaypipes commented 2 years ago

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 commented 2 years ago
  1. 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 commented 2 years ago

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

AaronME commented 2 years ago

@jaypipes I appreciate you taking a look at it.

muvaf commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@ack-bot: Closing this issue.

In response to [this](https://github.com/aws-controllers-k8s/community/issues/1078#issuecomment-1146700262): >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.