⚠️ feat: new features about support warning with webhook #2014

STRRL · 2022-09-29T06:44:50Z

Signed-off-by: STRRL im@strrl.dev

Two new interfaces ValidatorWarn and CustomValidatorWarn, for support warnings in validating webhooks.

k8s-ci-robot · 2022-09-29T06:44:58Z

Hi @STRRL. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

invidian

Thanks for your PR @STRRL!

I'm not familiar enough with project conventions to provide detailed feedback. From the library user perspective, one e2e test would be nice, with a code example how errors can be created, but I don't know if there are facilities prepared to have something like this and it's perhaps out of scope for this PR.

Overall it looks OK. Unfortunately, I don't have resources to test with with CABPK at the moment to verify e2e that the warnings produced are nicely showed by kubectl.

pkg/webhook/admission/validator_warn.go

camilamacedo86 · 2022-11-01T20:58:20Z

/ok-to-test

camilamacedo86 · 2022-11-01T21:06:26Z

/test pull-controller-runtime-test-master

STRRL · 2022-11-03T07:42:44Z

happy to see new progress on this PR; I have fixed the test cases and linter :)

PTAL @camilamacedo86

joelanford · 2022-11-08T21:50:48Z

Would this implementation effectively supersede the existing high-level validator interfaces? At first glance, it seems like it has identical functionality except that it can also return warnings?

I worry about maintaining both (almost identical) implementations over time.

I commented over on the issue, but I think we could use the existing interfaces and have a custom error type that includes the warnings like you suggest.

sbueringer · 2022-11-25T12:16:38Z

I worry about maintaining both (almost identical) implementations over time.
I commented over on the issue, but I think we could use the existing interfaces and have a custom error type that includes the warnings like you suggest.

I agree. We should definitely avoid introducing a third set of interfaces. I'll continue on the issue. Let's keep the discussion there for now to avoid concurrent threads.

STRRL · 2022-12-05T12:58:28Z

just mention the discussion result: we prefer to break the API instead of introducing a new type called ErrorWithWarnings, ref: #1896 (comment)

STRRL · 2022-12-05T13:14:53Z

Hi! I have updated this PR with these changes:

stop using goerrors "errors" in import
move error as the last value returned by methods, eg. ValidateCreate() ( err error, warnings []string) -> ValidateCreate() (warnings []string, err error)

STRRL · 2022-12-05T13:15:50Z

Hi! I have updated this PR with these changes:

stop using goerrors "errors" in import

move error as the last value returned by methods, eg. ValidateCreate() ( err error, warnings []string) -> ValidateCreate() (warnings []string, err error)

/cc @invidian @sbueringer

sbueringer · 2023-01-10T19:59:10Z

Thx sorry for the late response, will review soon

Signed-off-by: STRRL <im@strrl.dev>

k8s-ci-robot · 2023-04-17T07:25:59Z

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

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	`b2a9552`	link	false	`/test pull-controller-runtime-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

sbueringer · 2023-04-17T13:00:29Z

Looks good thx!

@STRRL can you please squash the commits?

/lgtm

sbueringer · 2023-04-17T13:01:09Z

/assign @vincepri

vincepri · 2023-04-19T13:58:42Z

/approve

k8s-ci-robot · 2023-04-19T13:59:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: STRRL, vincepri

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:

~~OWNERS~~ [vincepri]

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

vincepri · 2023-04-19T14:22:34Z

/hold cancel

- controller runtime v0.15 contains breaking changes where Validator interfaces have been modified to include warning in return types: kubernetes-sigs/controller-runtime#2014 other breaking changes do not affect us

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

…es to return warnings kubernetes-sigs/controller-runtime#2014

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 29, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 29, 2022

k8s-ci-robot requested review from sbueringer and varshaprasad96 September 29, 2022 06:45

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 29, 2022

invidian reviewed Oct 5, 2022

View reviewed changes

pkg/webhook/admission/validator_warn.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 1, 2022

STRRL force-pushed the webhook-warning branch 2 times, most recently from 6f227af to f246cc3 Compare November 3, 2022 04:24

a-cordier mentioned this pull request Nov 4, 2022

feat: admission webhooks gravitee-io/gravitee-kubernetes-operator#136

Closed

fabriziopandini mentioned this pull request Nov 30, 2022

Enforce good selectors on MachineDeployments kubernetes-sigs/cluster-api#6283

Closed

STRRL requested review from invidian and sbueringer and removed request for sbueringer, varshaprasad96 and invidian December 5, 2022 13:12

k8s-ci-robot requested a review from sbueringer December 5, 2022 13:15

chore: warning message should also be carried with APIStatusError

b2a9552

Signed-off-by: STRRL <im@strrl.dev>

k8s-ci-robot requested a review from sbueringer April 17, 2023 07:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2023

k8s-ci-robot merged commit 1c2a7c9 into kubernetes-sigs:main Apr 19, 2023

sbueringer mentioned this pull request Apr 22, 2023

Support warnings in webhook utils #1788

Closed

vivek-koppuru mentioned this pull request May 25, 2023

Add warning to deprecate disableCSI through CLI aws/eks-anywhere#5918

Merged

masa213f mentioned this pull request May 29, 2023

Support Kubernetes 1.27 cybozu-go/moco#525

Merged

ChunyiLyu mentioned this pull request May 30, 2023

Bump controller runtime and k8s api 1.27 rabbitmq/messaging-topology-operator#628

Merged

ary1992 mentioned this pull request Jun 23, 2023

Upgrade k8s.io/* to v0.27, sigs.k8s.io/controller-runtime to v0.15 gardener/gardener#7991

Closed

ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023

Adapt webhook/admission custom Validator and CustomValidator interfac…

5fffead

…es to return warnings kubernetes-sigs/controller-runtime#2014

Sunnatillo mentioned this pull request Aug 1, 2023

⚠️ Bump CAPI to v1.5.0 metal3-io/cluster-api-provider-metal3#1079

Merged

lyarwood mentioned this pull request Aug 9, 2023

chore: Update controller-runtime to v0.15.0 kubevirt/ssp-operator#604

Closed

erkanerol mentioned this pull request Aug 21, 2023

Bump sigs.k8s.io/controller-runtime from 0.13.1 to 0.15.1 giantswarm/cluster-api-cleaner-vsphere#29

Closed

nakamasato mentioned this pull request Sep 27, 2023

docs: update webhook nakamasato/kubernetes-training#300

Merged

nakamasato mentioned this pull request Nov 23, 2023

fix: webhook validate logic in kubebuilder version 3.11.0 nakamasato/password-operator#27

Merged

troy0820 mentioned this pull request Dec 1, 2023

Consider stabilizing the API and moving to v1 and beyond #2327

Closed

STRRL deleted the webhook-warning branch October 14, 2024 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ feat: new features about support warning with webhook #2014

⚠️ feat: new features about support warning with webhook #2014

STRRL commented Sep 29, 2022

k8s-ci-robot commented Sep 29, 2022

invidian left a comment •

edited

Loading

camilamacedo86 commented Nov 1, 2022

camilamacedo86 commented Nov 1, 2022

STRRL commented Nov 3, 2022

joelanford commented Nov 8, 2022

sbueringer commented Nov 25, 2022 •

edited

Loading

STRRL commented Dec 5, 2022 •

edited

Loading

STRRL commented Dec 5, 2022 •

edited

Loading

STRRL commented Dec 5, 2022

sbueringer commented Jan 10, 2023

k8s-ci-robot commented Apr 17, 2023

sbueringer commented Apr 17, 2023

sbueringer commented Apr 17, 2023

vincepri commented Apr 19, 2023

k8s-ci-robot commented Apr 19, 2023

vincepri commented Apr 19, 2023

⚠️ feat: new features about support warning with webhook #2014

⚠️ feat: new features about support warning with webhook #2014

Conversation

STRRL commented Sep 29, 2022

k8s-ci-robot commented Sep 29, 2022

invidian left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 commented Nov 1, 2022

camilamacedo86 commented Nov 1, 2022

STRRL commented Nov 3, 2022

joelanford commented Nov 8, 2022

sbueringer commented Nov 25, 2022 • edited Loading

STRRL commented Dec 5, 2022 • edited Loading

STRRL commented Dec 5, 2022 • edited Loading

STRRL commented Dec 5, 2022

sbueringer commented Jan 10, 2023

k8s-ci-robot commented Apr 17, 2023

sbueringer commented Apr 17, 2023

sbueringer commented Apr 17, 2023

vincepri commented Apr 19, 2023

k8s-ci-robot commented Apr 19, 2023

vincepri commented Apr 19, 2023

invidian left a comment •

edited

Loading

sbueringer commented Nov 25, 2022 •

edited

Loading

STRRL commented Dec 5, 2022 •

edited

Loading

STRRL commented Dec 5, 2022 •

edited

Loading