Skip to content

feat: return error on missing required fields #613

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: return error on missing required fields #613

wants to merge 1 commit into from

Conversation

michaelhtm
Copy link
Member

Issue #2557

Description of changes:
During resource Adoption, the ACK controllers only returned an error
when there was a missing PrimaryKey. If a Resource requires more than 1
field to describe a Resource (eg. Lambda alias requires Name and
FunctionName), the controller only verifies the Primary identifier (ARN,
ID, Name) and nothing else.

With this change, we look into the SDK model to retrieve the required
fields and return an error if all are not defined during
PopulateResourceFromAnnotation

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

@ack-prow ack-prow bot requested review from a-hilaly and knottnt July 25, 2025 17:21
@ack-prow ack-prow bot added the approved label Jul 25, 2025
@michaelhtm
Copy link
Member Author

/retest

a-hilaly
a-hilaly reviewed Jul 25, 2025
View reviewed changes
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.

noice! thanks @michaelhtm

pkg/generate/code/set_resource.go Outdated
@@ -847,6 +847,7 @@ func identifierNameOrIDGuardConstructor(
// return ackerrors.MissingNameIdentifier
// }
func requiredFieldGuardContructor(
holdingVar string,
Copy link
Member

Choose a reason for hiding this comment

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

what does holdingVar mean? does it need to be the first param?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's just the var name for the requiredKey..

pkg/generate/code/set_resource.go
@@ -847,6 +847,7 @@ func identifierNameOrIDGuardConstructor(
// return ackerrors.MissingNameIdentifier
// }
Copy link
Member

Choose a reason for hiding this comment

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

maybe this comment needs to be updated?

pkg/generate/code/set_resource.go Outdated
Comment on lines 1545 to 1549
// The variable used for primary key
requiredFieldVarName string,
Copy link
Member

Choose a reason for hiding this comment

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

is this variable needed a this level? doc updates?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we construct it inside the PopulateResourceFromAnnotation, where we find out if a field is a primary key or it's just a required field.

Copy link
Member Author

Choose a reason for hiding this comment

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

doc updates?

@michaelhtm michaelhtm force-pushed the chore/returnerroronmissingoptionalfields branch from 831f5ac to 5e72da5 Compare July 25, 2025 18:03
knottnt
knottnt reviewed Jul 25, 2025
View reviewed changes
Copy link
Contributor

@knottnt knottnt left a comment

Choose a reason for hiding this comment

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

Q: Is a field being required always a field we need to adopt a resource? It seems like a field could be considered required to create/update a resource, but not necessarily to identify it.

@michaelhtm
feat: return error on missing required fields
6f0d5cd 
During resource Adoption, the ACK controllers only returned an error
when there was a missing PrimaryKey. If a Resource requires more than 1
field to describe a Resource (eg. Lambda alias requires Name and
FunctionName), the controller only verifies the Primary identifier (ARN,
ID, Name) and nothing else.

With this change, we look into the SDK model to retrieve the required
fields and return an error if all are not defined during
`PopulateResourceFromAnnotation`
@michaelhtm michaelhtm force-pushed the chore/returnerroronmissingoptionalfields branch from 5e72da5 to 6f0d5cd Compare August 4, 2025 17:57
@michaelhtm michaelhtm requested a review from knottnt August 4, 2025 17:57
a-hilaly
a-hilaly reviewed Aug 4, 2025
View reviewed changes
pkg/generate/code/set_resource.go
// if !ok {
// return ackerrors.MissingNameIdentifier
Copy link
Member

Choose a reason for hiding this comment

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

how about return ackerrors.MissingNameIdentifier(fieldNames...)

Copy link
Member

Choose a reason for hiding this comment

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

So for a longtime this wasn't considered a terminal error?

Copy link
Member Author

Choose a reason for hiding this comment

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

MissingNameIdentifier is an error introduced by SetIdentifiers (Adopted CRD). It was mainly used when NameOrID was nil.
This error message fix was introduced here

knottnt
knottnt approved these changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@knottnt knottnt left a comment

Choose a reason for hiding this comment

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

@michaelhtm thanks for resolving my. Holding of on lgtm so that @a-hilaly can verify that their comments have been resolved.

@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt, michaelhtm

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

@michaelhtm
Copy link
Member Author

/retest

@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 5, 2025

@michaelhtm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
verify-attribution 6f0d5cd link false /test verify-attribution
s3-olm-test 6f0d5cd link false /test s3-olm-test
eks-controller-test 6f0d5cd link true /test eks-controller-test

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-hilaly a-hilaly a-hilaly left review comments

@knottnt knottnt knottnt approved these changes

Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelhtm @a-hilaly @knottnt