arrayofstruct: follow-up on review #166
Conversation
- Exported IsFieldRequired utility function - Updated hasRequiredField to use the exported utility - Added recursion exit condition for basic types - Updated error message to avoid specifying exact marker Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
cncf-cla: yes
|
/assign @everettraven |
saschagrunert
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| } | ||
|
|
||
| message := fmt.Sprintf("%s is an array of structs, but the struct has no required fields. At least one field should be marked as %s to prevent ambiguous YAML configurations", prefix, markers.RequiredMarker) | ||
| message := fmt.Sprintf("%s is an array of structs, but the struct has no required fields. At least one field should be marked as required to prevent ambiguous YAML configurations", prefix) |
There was a problem hiding this comment.
Do we need test updates with this change? I'd suspect we do.
There was a problem hiding this comment.
Added two new test cases for that change in 2993091
There was a problem hiding this comment.
I don't think this change is enough because I can't see this message as want in test-cases yet. You just added valid case, but we also want invalid case of this message.
There was a problem hiding this comment.
Yeah we can probably re-use the existing cases and extend them with the error message. 👍
k8s-ci-robot
added
size/M
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert
force-pushed
the
arrayofstruct-followup
branch
from
2993091 to
527590e
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, saschagrunert 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 |
k8s-ci-robot
added
the
approved
| // IsFieldRequired checks if the field is required. | ||
| // It checks for the presence of the required marker, the kubebuilder required marker, or the k8s required marker. | ||
| func isFieldRequired(field *ast.Field, markersAccess markershelper.Markers) bool { | ||
| func IsFieldRequired(field *ast.Field, markersAccess markershelper.Markers) bool { | ||
| fieldMarkers := markersAccess.FieldMarkers(field) | ||
|
|
||
| return fieldMarkers.Has(markers.RequiredMarker) || |
There was a problem hiding this comment.
I think I saw this change get merged recently as part of another change. You may need to rebase to pick this up.
5 participants
Follow-up on #162