Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend code generator set functionality/directive #464

Merged
merged 6 commits into from
Apr 8, 2024

Conversation

Vandita2020
Copy link
Member

@Vandita2020 Vandita2020 commented Sep 5, 2023

Issue
This PR is related to fix the issue generating after executing PR #462

Summary
The PR helps in disregarding fields marked with set: ignore for nested fields. It also enhances the functionality of set:ignore to support the exclusion of fields from set_sdk.

Description

The PR fixes the issue coming after a new custom field is created and has set.ignore set to true for it. Even after setting the field to ignore, the field is getting used to set the SDK fields. In this case particularly, the new field was defined as shown below:

Code.S3SHA256:
        type: string
        set:  
          - method: Create
            ignore: true

And the issue it is causing is under sdk.go/newCreateRequestPayload function, where it is being set, as shown below:

if r.ko.Spec.Code.S3SHA256 != nil {
	f1.SetS3SHA256(*r.ko.Spec.Code.S3SHA256)
}

The goal is to ignore this particular line of code. And also to improve the basic functionality of set.ignore.

The set.ignore functionality was till now supporting exclusion of fields only when the field was used to set for the resource. This is when the set.from is also given, the set.from ignores the field coming from the resource. However when set.to is used, which is to ignore the field while setting SDK fields, the ignore didn't support that. This functionality got left behind because the set.to was added afterwards.

This PR added new way to set ignore. The ignore field can be set to from, to or all depending on whether you want to ignore setting the field for resource, or ignore the field for sdk, or for both, respectively.

So the above code will now look like:

Code.S3SHA256:
        type: string
        set:
        - method: Create
           ignore: "to"

Limitations
Currently this PR checks the set.ignore field only for members of the field whose type is structure.

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

@ack-prow
Copy link

ack-prow bot commented Sep 5, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ack-prow ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2023
@Vandita2020 Vandita2020 marked this pull request as ready for review September 5, 2023 23:26
@ack-prow ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2023
@a-hilaly
Copy link
Member

a-hilaly commented Sep 6, 2023

/retest

@a-hilaly
Copy link
Member

/test all

@Vandita2020
Copy link
Member Author

/test dynamodb-controller-test

1 similar comment
@Vandita2020
Copy link
Member Author

/test dynamodb-controller-test

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Thank you @Vandita2020 - looks like this PR have some changes that we reviewd in #462 . Can you please delete the first 4 commits in this branch?
/hold

@ack-prow ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023
@Vandita2020 Vandita2020 force-pushed the ignore_setS3SHA branch 5 times, most recently from 50067ec to 4b27e88 Compare September 28, 2023 20:03
@Vandita2020
Copy link
Member Author

/test s3-controller-test

@Vandita2020
Copy link
Member Author

/test ec2-controller-test

@a-hilaly
Copy link
Member

/test all

@a-hilaly
Copy link
Member

/retest all

Copy link

ack-prow bot commented Feb 27, 2024

@a-hilaly: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test acm-controller-test
  • /test apigatewayv2-controller-test
  • /test cloudfront-controller-test
  • /test documentdb-controller-test
  • /test dynamodb-controller-test
  • /test ec2-controller-test
  • /test ecr-controller-test
  • /test efs-controller-test
  • /test eks-controller-test
  • /test eventbridge-controller-test
  • /test iam-controller-test
  • /test lambda-controller-test
  • /test pipes-controller-test
  • /test prometheusservice-controller-test
  • /test s3-controller-test
  • /test unit-test

The following commands are available to trigger optional jobs:

  • /test s3-olm-test

Use /test all to run all jobs.

In response to this:

/retest all

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.

@a-hilaly
Copy link
Member

/test all

@a-hilaly
Copy link
Member

/retest

@a-hilaly
Copy link
Member

/test all

1 similar comment
@a-hilaly
Copy link
Member

a-hilaly commented Mar 1, 2024

/test all

@a-hilaly
Copy link
Member

a-hilaly commented Apr 2, 2024

/retest

@Vandita2020
Copy link
Member Author

/test s3-olm-test

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Great stuff~
/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024
Copy link

ack-prow bot commented Apr 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, Vandita2020

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:

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

@ack-prow ack-prow bot added the approved label Apr 8, 2024
@a-hilaly
Copy link
Member

a-hilaly commented Apr 8, 2024

/unhold

@ack-prow ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2024
@ack-prow ack-prow bot merged commit 37f4ba2 into aws-controllers-k8s:main Apr 8, 2024
18 checks passed
@a-hilaly a-hilaly changed the title Functionality to make ignore work Extend ignore directive functionality Apr 8, 2024
@a-hilaly a-hilaly changed the title Extend ignore directive functionality Extend code generator set functionality/directive Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants