Error aggregation for Validation Webhooks #154
Conversation
|
Hi @k-keiichi-rh. Thanks for your PR. I'm waiting for a medik8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k-keiichi-rh
force-pushed
the
implement-aggregatederror
branch
from
7f1e148
to
1cd836a
Compare
|
/ok-to-test |
|
/retest |
| validateStrategy(farSpec.RemediationStrategy), | ||
| }) | ||
|
|
||
| return nil, aggregated |
There was a problem hiding this comment.
NIT: unrelated to this change but it would be clearer to return an empty warning instead of a nil.
| return nil, aggregated | |
| return admission.Warnings{}, aggregated |
There was a problem hiding this comment.
Thank you for the suggestion. I will fix it as you mentioned.
After applying this change, I got the following error in the unit tests:
> Enter [It] should be denied - /home/kii/git/kii/fence-agents-remediation/api/v1alpha1/fenceagentsremediation_webhook_test.go:50 @ 09/22/24 09:23:38.584
2024-09-22T09:23:38-04:00 INFO fenceagentsremediation-resource validate create {"name": "test-fence_ipmilan"}
[FAILED] Unexpected non-nil/non-zero argument at index 0:
<admission.Warnings>: admission.Warnings{}
In [It] at: /home/kii/git/kii/fence-agents-remediation/vendor/github.com/onsi/gomega/internal/assertion.go:62 @ 09/22/24 09:23:38.584
I can simply avoid this error with the following:
[Before]
Expect(outOfServiceStrategy.ValidateCreate()).Error().To(MatchError(ContainSubstring(outOfServiceTaintUnsupportedMsg)))
[After]
_, err := outOfServiceStrategy.ValidateCreate()
Expect(err).To(MatchError(ContainSubstring(outOfServiceTaintUnsupportedMsg)))
But I think it's not a smart fix to avoid this error. Is there another good way to fix?
There was a problem hiding this comment.
Is there another good way to fix?
We can simply test whether the admission warning is empty
warnings, err := outOfServiceStrategy.ValidateCreate()
ExpectWithOffset(1, warnings).To(BeEmpty())
Expect(err).To(MatchError(ContainSubstring(outOfServiceTaintUnsupportedMsg)))
There was a problem hiding this comment.
Thank you for the comment. I fixed it as you suggested.
k-keiichi-rh
force-pushed
the
implement-aggregatederror
branch
from
1cd836a
to
caf5889
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: k-keiichi-rh, razo7 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 |
|
/retest |
|
/test 4.16-openshift-e2e |
|
/retest |
|
/test 4.16-openshift-e2e |
|
/test 4.16-openshift-e2e |
2 similar comments
|
/test 4.16-openshift-e2e |
|
/test 4.16-openshift-e2e |
|
/retest |
|
Trying again after #159 changes |
Why we need this PR
Refactoring for Validation Webhooks with error aggregation
This is suggested here.
Changes made
Run all validations and return an aggregated error