refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace #1241

zdtsw · 2024-09-17T09:58:45Z

Description

use Default() instead of setDSCDefaults()

How Has This Been Tested?

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

ykaliuta · 2024-09-17T11:32:39Z

controllers/webhook/webhook.go

@@ -44,10 +47,10 @@ type OpenDataHubValidatingWebhook struct {
 	Decoder *admission.Decoder
 }

-func (w *OpenDataHubValidatingWebhook) SetupWithManager(mgr ctrl.Manager) {
+func (v *OpenDataHubValidatingWebhook) SetupValidatingWebhookWithManager(mgr ctrl.Manager) {


Why?
They have distinct types and type.SetupWithManager is self describing

ykaliuta · 2024-09-17T11:34:17Z

controllers/webhook/webhook.go

@@ -76,7 +79,7 @@ func denyCountGtZero(ctx context.Context, cli client.Client, gvk schema.GroupVer
 	return admission.Allowed("")
 }

-func (w *OpenDataHubValidatingWebhook) checkDupCreation(ctx context.Context, req admission.Request) admission.Response {
+func (v *OpenDataHubValidatingWebhook) checkDupCreation(ctx context.Context, req admission.Request) admission.Response {


why? w as webhook is good enough. (I would abstract it even further till s Self :)

right, forgot to revert this change after commit 2

ykaliuta · 2024-09-17T11:34:58Z

controllers/webhook/webhook.go

+		return admission.Errored(http.StatusInternalServerError, err)
+	}
+	// use PatchResponseFromRaw to ensure dsc CR is updated with defaults
+	return admission.PatchResponseFromRaw(req.Object.Raw, dscBytes)


so finally this is the actual change :) But still, could you put some reasoning behind?

this is outdated change, check the latest code.

bartoszmajsak · 2024-09-18T08:38:51Z

controllers/webhook/webhook.go

 }

-func (w *OpenDataHubMutatingWebhook) setDSCDefaults(_ context.Context, dsc *dscv1.DataScienceCluster) admission.Response {
+// implement admission.CustomDefaulter interface.
+func (m *DSCDefaulter) Default(_ context.Context, obj runtime.Object) error {


As this is now extracted to a simple function, how about providing some unit tests for it? It does not even have to employ EnvTest, plain unit testing should be fine here.

did some updates on the unit-test.
only for the case which mutate webhook is related.

…namespace - rename SetupValidatingWebhookWithManager and SetupMutateWebhookWithManager per each - use Default() instead of setDSCDefaults() - return call PatchResponseFromRaw than admission.Allowed() Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

… is "" - update unit-test Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw · 2024-09-19T12:59:29Z

/test opendatahub-operator-e2e

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci · 2024-09-19T15:59:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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~~ [ykaliuta]

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

…ModelReg namespace (red-hat-data-services#1241) * refactor: change for mutating webhook to use Default to set ModelReg namespace - rename SetupValidatingWebhookWithManager and SetupMutateWebhookWithManager per each - use Default() instead of setDSCDefaults() - return call PatchResponseFromRaw than admission.Allowed() Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rework implmentation Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: revert some unnecessary changes and add type in log Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: make it more clear for the "defaulter" case is when namespace is "" - update unit-test Signed-off-by: Wen Zhou <wenzhou@redhat.com> * test: add more testcase Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rename function and move comments Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit d33ccf6)

…ModelReg namespace (#1241) * refactor: change for mutating webhook to use Default to set ModelReg namespace - rename SetupValidatingWebhookWithManager and SetupMutateWebhookWithManager per each - use Default() instead of setDSCDefaults() - return call PatchResponseFromRaw than admission.Allowed() Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rework implmentation Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: revert some unnecessary changes and add type in log Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: make it more clear for the "defaulter" case is when namespace is "" - update unit-test Signed-off-by: Wen Zhou <wenzhou@redhat.com> * test: add more testcase Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rename function and move comments Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit d33ccf6)

openshift-ci bot requested review from CFSNM and StevenTobin September 17, 2024 09:58

zdtsw requested review from bartoszmajsak and ykaliuta and removed request for StevenTobin and CFSNM September 17, 2024 09:59

zdtsw changed the title ~~refactor: change for mutating webhook to use Default to set ModelReg namespace~~ refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace Sep 17, 2024

zdtsw marked this pull request as draft September 17, 2024 10:20

openshift-ci bot added the do-not-merge/work-in-progress label Sep 17, 2024

ykaliuta reviewed Sep 17, 2024

View reviewed changes

zdtsw marked this pull request as ready for review September 17, 2024 13:14

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 17, 2024

openshift-ci bot requested review from adelton and AjayJagan September 17, 2024 13:14

zdtsw removed request for adelton and AjayJagan September 17, 2024 13:14

bartoszmajsak reviewed Sep 18, 2024

View reviewed changes

zdtsw added 3 commits September 18, 2024 16:15

update: rework implmentation

4fec71b

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

update: revert some unnecessary changes and add type in log

ad31f05

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw force-pushed the chore_117 branch from a296d19 to 5613b61 Compare September 18, 2024 15:41

update: make it more clear for the "defaulter" case is when namespace…

469750b

… is "" - update unit-test Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw force-pushed the chore_117 branch from 5613b61 to 469750b Compare September 18, 2024 15:43

test: add more testcase

3716db9

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw requested review from bartoszmajsak and ykaliuta September 19, 2024 10:40

update: rename function and move comments

9507a1d

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta approved these changes Sep 19, 2024

View reviewed changes

openshift-ci bot assigned ykaliuta Sep 19, 2024

openshift-ci bot added the lgtm label Sep 19, 2024

openshift-ci bot added the approved label Sep 19, 2024

openshift-merge-bot bot merged commit d33ccf6 into opendatahub-io:incubation Sep 19, 2024
8 checks passed

zdtsw mentioned this pull request Oct 1, 2024

[sync]: refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace red-hat-data-services/rhods-operator#365

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace #1241

refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace #1241

zdtsw commented Sep 17, 2024 •

edited

Loading

ykaliuta Sep 17, 2024

ykaliuta Sep 17, 2024

zdtsw Sep 17, 2024

ykaliuta Sep 17, 2024

zdtsw Sep 17, 2024

bartoszmajsak Sep 18, 2024

zdtsw Sep 19, 2024

zdtsw commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace #1241

refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace #1241

Conversation

zdtsw commented Sep 17, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

ykaliuta Sep 17, 2024

Choose a reason for hiding this comment

ykaliuta Sep 17, 2024

Choose a reason for hiding this comment

zdtsw Sep 17, 2024

Choose a reason for hiding this comment

ykaliuta Sep 17, 2024

Choose a reason for hiding this comment

zdtsw Sep 17, 2024

Choose a reason for hiding this comment

bartoszmajsak Sep 18, 2024

Choose a reason for hiding this comment

zdtsw Sep 19, 2024

Choose a reason for hiding this comment

zdtsw commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

zdtsw commented Sep 17, 2024 •

edited

Loading