-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add OmitUnchangedField
config for selective inclusion of changed CRD
fields in update requests.
#406
Add OmitUnchangedField
config for selective inclusion of changed CRD
fields in update requests.
#406
Conversation
pkg/generate/code/set_sdk.go
Outdated
// | ||
// .Authors ([]*string) | ||
// .ImageData | ||
// .Location (*string) | ||
// .Tag (*string) | ||
// .Name (*string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird from my new gofmt (using go1.20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, no worries. we should probably do a single style-only commit that updates gofmt for this repository (and runtime)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, no worries. we should probably do a single style-only commit that updates gofmt for this repository (and runtime)
@a-hilaly et voila:
2c7f73d
to
ea71010
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. One little suggestion inline but I approve of this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to +1 Jay's comment, otherwise I think it's ready to ship!
ea71010
to
c0dc480
Compare
fields in update requests. This patch adds a new generator configuration `update_operation.omit_unchanged_fields` that instructs the code generator to generate cvode that only include fields in update reuqests if their values have actually changed. This is accomplished by using `delta.DifferentAt` before setting any field in `newUpdateRequest` function. This change will improve the reliability of the generated code by preventing the controller from trying to update unecessary fields. Signed-off-by: Amine Hilaly <hilalyamine@gmail.com>
c0dc480
to
e248468
Compare
/test all |
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
@a-hilaly: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Addressed the comment anyone can manually ship this? @RedbackThomson @jljaco |
Ready to ship. Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is not introducing the failure in the ec2 test:
Error: template: /home/prow/go/src/github.com/aws-controllers-k8s/code-generator/templates/pkg/resource/references.go.tpl:120:3: executing "/home/prow/go/src/github.com/aws-controllers-k8s/code-generator/templates/pkg/resource/references.go.tpl" at <GoCodeResolveReference $field "ko" 1>: error calling GoCodeResolveReference: references within lists inside lists aren't supported. crd: "RouteTable", path: "Routes.GatewayID"
make: *** [Makefile:41: build-controller] Error 2
I recommend we force-merge this patch and fix the nested list reference struct issue separately.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, 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:
Approvers can indicate their approval by writing |
Issue: aws-controllers-k8s/community#1598
This patch adds a new generator configuration
update_operation.omit_unchanged_fields
that instructs the codegenerator to generate code that only includes fields in update requests
if their values have actually changed. This is accomplished by using
delta.DifferentAt
before setting any field in thenewUpdateRequest
function.
This change will improve the reliability of the generated code by
preventing the controller from trying to update unnecessary fields.
Signed-off-by: Amine Hilaly hilalyamine@gmail.com
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.