-
Notifications
You must be signed in to change notification settings - Fork 835
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.
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
remove mutating webhook from Seldon Core Operator #2852
remove mutating webhook from Seldon Core Operator #2852
Conversation
Mon Jan 18 18:53:55 UTC 2021 impatient try |
Mon Jan 18 18:54:11 UTC 2021 impatient try |
Mon Jan 18 18:57:03 UTC 2021 impatient try |
Mon Jan 18 18:57:16 UTC 2021 impatient try |
Mon Jan 18 18:59:25 UTC 2021 impatient try |
Mon Jan 18 18:59:32 UTC 2021 impatient try |
/test integration |
Tue Jan 19 10:38:43 UTC 2021 impatient try |
Tue Jan 19 10:38:42 UTC 2021 impatient try |
Tue Jan 19 10:38:40 UTC 2021 impatient try |
Tue Jan 19 10:38:48 UTC 2021 impatient try |
@@ -540,18 +547,6 @@ func (r *SeldonDeploymentSpec) ValidateSeldonDeployment() error { | |||
|
|||
// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! | |||
|
|||
// +kubebuilder:webhook:path=/mutate-machinelearning-seldon-io-v1-seldondeployment,mutating=true,failurePolicy=fail,sideEffects=None,groups=machinelearning.seldon.io,resources=seldondeployments,verbs=create;update,versions=v1,name=v1.mseldondeployment.kb.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code related to the webhook should be removed as well, as there are several functions in this file which would no longer be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep the mutating functionality which is called from Reconcile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense, although it woudl still be worth moving it from the webhook file as it would no longer be used by the webhook itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the code will be required in the webhooks -> validating webhooks were receiving defaulted resource before.
Now they need to apply defaulting on their own before validating -> I suspect that was the reason why tests failed in previous run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now moved into a file containing actual types.
Seems integration tests are not going well
gonna have to look locally into them |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Let's hope it is fixed now. |
Tue Jan 19 12:04:01 UTC 2021 impatient try |
Tue Jan 19 12:04:05 UTC 2021 impatient try |
Tue Jan 19 12:06:38 UTC 2021 impatient try |
Only 13 failures now... |
/test integration |
Tue Jan 19 15:47:26 UTC 2021 impatient try |
Tue Jan 19 15:47:40 UTC 2021 impatient try |
Tue Jan 19 15:47:43 UTC 2021 impatient try |
Fri Jan 22 12:10:19 UTC 2021 impatient try |
had to rebase on master |
Fri Jan 22 14:10:26 UTC 2021 impatient try |
Fri Jan 22 14:10:27 UTC 2021 impatient try |
Fri Jan 22 14:10:35 UTC 2021 impatient try |
Fri Jan 22 14:10:37 UTC 2021 impatient try |
/retest |
Fri Jan 22 18:14:53 UTC 2021 impatient try |
something had to go wrong during rebase, gonna fix on Monday |
/retest |
Fri Jan 22 20:42:46 UTC 2021 impatient try |
/retest |
Sat Jan 23 17:14:22 UTC 2021 impatient try |
/retest |
Mon Jan 25 09:41:04 UTC 2021 impatient try |
/test integration |
Tue Jan 26 14:01:07 UTC 2021 impatient try |
Tue Jan 26 14:01:22 UTC 2021 impatient try |
Tue Jan 26 14:02:48 UTC 2021 impatient try |
Tue Jan 26 14:02:59 UTC 2021 impatient try |
Rebased onto master, nicely squashed commit and also tested once again locally that mutatingwebhook is not getting created. @cliveseldon, don't know if you want to have a final look over the changes? |
Hmm... notebook tests failed on installation of istio
/retest |
Tue Jan 26 15:08:49 UTC 2021 impatient try |
/retest |
Tue Jan 26 16:41:28 UTC 2021 impatient try |
All testes passed!!! Let's land this 🚀 |
Nice one |
What this PR does / why we need it:
This removes the mutating webhook:
Mutatingwebhookconfigurations
is not installed into the cluster.From now on, the
SeldonDeployment
resource written toetcd
of k8s will be exactly the same as resource applied by the user.This will simplify GitOps operations as the parity between applied and visible resources will be present.
To see extra information that was written into CR before one can take a look at the generated deployments.
They will contain all information about the assigned ports, init containers, etc.
Which issue(s) this PR fixes:
Closes #2817
Closes #2811
Special notes for your reviewer:
Does this PR introduce a user-facing change?: