Skip to content
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

Support objectSelector on mutating webhook #2058

Merged
merged 12 commits into from
Jun 17, 2024

Conversation

Cian911
Copy link
Contributor

@Cian911 Cian911 commented Jun 13, 2024

🛑 Important:

Please open an issue to discuss significant work before you start. We appreciate your contributions and don't want your efforts to go to waste!

For guidelines on how to contribute, please review the CONTRIBUTING.md document.

Purpose of this PR

When the jobs are run for the spark-operator in the same namespace in which the operator resides, the operator will attempt to mutate itself.

Proposed changes:

  • This PR adds support for specifying an objectSelector on the MutatingWebhookConfiguration so as a user can specify what pods matching a given set of labels the operator can mutate. See also related issue requesting this.
  • This PR also updates the minikube CI action to the latest release. This has been moved to this PR.

Change Category

Indicate the type of change by marking the applicable boxes:

  • Bugfix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that could affect existing functionality)
  • Documentation update

Rationale

Checklist

Before submitting your PR, please review the following:

  • I have conducted a self-review of my own code.
  • I have updated documentation accordingly.
  • I have added tests that prove my changes are effective or that my feature works.
  • Existing unit tests pass locally with my changes.

Additional Notes

@Cian911
Copy link
Contributor Author

Cian911 commented Jun 13, 2024

Looks like the CI failures caused as a result of minikube kubernetes/minikube#18021

@Cian911 Cian911 marked this pull request as ready for review June 13, 2024 10:54
@google-oss-prow google-oss-prow bot requested a review from andreyvelich June 13, 2024 10:55
@yuchaoran2011
Copy link
Contributor

This is a great addition. But can we limit each PR to a specific goal only (e.g. moving the CI action change to another PR)? That way, it's easier to troubleshoot and revert if needed.

@Cian911
Copy link
Contributor Author

Cian911 commented Jun 13, 2024

This is a great addition. But can we limit each PR to a specific goal only (e.g. moving the CI action change to another PR)? That way, it's easier to troubleshoot and revert if needed.

Sure thing, I can move the CI change to a different PR. I just made the change to ensure the CI would pass.

I'll do that now, which will need to be merged in first 👍

@Cian911 Cian911 mentioned this pull request Jun 13, 2024
8 tasks
@Cian911
Copy link
Contributor Author

Cian911 commented Jun 13, 2024

@yuchaoran2011 I have removed the change from this PR (hence why some actions are failing), and moved it to this PR. This new PR will need to be merged first, then this one rebased afterwards.

@vikas-saxena02
Copy link
Contributor

@yuchaoran2011 I have removed the change from this PR (hence why some actions are failing), and moved it to this PR. This new PR will need to be merged first, then this one rebased afterwards.

Thanks @Cian911 I was just about to open another bug as the current CI is failing. @yuchaoran2011 @andreyvelich @vara-bonthu can we please merge this PR on a priority

Signed-off-by: Cian Gallagher <[email protected]>
@Cian911
Copy link
Contributor Author

Cian911 commented Jun 15, 2024

@yuchaoran2011 @vara-bonthu Now that my other PR has been merged with regards to the minikube CI issue, this PR should be good to go, if I can get a review/approval 🙏

@vara-bonthu
Copy link
Contributor

PR looks good to me. Could you confirm if your local tests are successful?

@Cian911
Copy link
Contributor Author

Cian911 commented Jun 17, 2024

PR looks good to me. Could you confirm if your local tests are successful?

Yes indeed, we are already using this in our clusters, see below:

Screenshot 2024-06-17 at 10 46 43

I've also just updated the chart version & docs again following the most recent merge. It would be great if any of you can review this again 🙇 cc: @vara-bonthu @yuchaoran2011

Copy link
Contributor

@yuchaoran2011 yuchaoran2011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuchaoran2011

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 4774fec into kubeflow:master Jun 17, 2024
7 checks passed
jacobsalway pushed a commit to jacobsalway/spark-operator that referenced this pull request Jul 25, 2024
* feat: add support for setting objectSelector on webhook

Signed-off-by: Cian Gallagher <[email protected]>

* feat: update objectSelector to match expressions

Signed-off-by: Cian Gallagher <[email protected]>

* chore: use out of the box label parser

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update app version

Signed-off-by: Cian Gallagher <[email protected]>

* fix: use parseSelector

Signed-off-by: Cian Gallagher <[email protected]>

* ci: update minikube action to latest release

Signed-off-by: Cian Gallagher <[email protected]>

* revert: undo ci changes. create seperate pr

Signed-off-by: Cian Gallagher <[email protected]>

* Trigger CI

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version & docs following previous merge

Signed-off-by: Cian Gallagher <[email protected]>

* docs: update docs

Signed-off-by: Cian Gallagher <[email protected]>

---------

Signed-off-by: Cian Gallagher <[email protected]>
sigmarkarl pushed a commit to spotinst/spark-on-k8s-operator that referenced this pull request Aug 7, 2024
* feat: add support for setting objectSelector on webhook

Signed-off-by: Cian Gallagher <[email protected]>

* feat: update objectSelector to match expressions

Signed-off-by: Cian Gallagher <[email protected]>

* chore: use out of the box label parser

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update app version

Signed-off-by: Cian Gallagher <[email protected]>

* fix: use parseSelector

Signed-off-by: Cian Gallagher <[email protected]>

* ci: update minikube action to latest release

Signed-off-by: Cian Gallagher <[email protected]>

* revert: undo ci changes. create seperate pr

Signed-off-by: Cian Gallagher <[email protected]>

* Trigger CI

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version & docs following previous merge

Signed-off-by: Cian Gallagher <[email protected]>

* docs: update docs

Signed-off-by: Cian Gallagher <[email protected]>

---------

Signed-off-by: Cian Gallagher <[email protected]>
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
* feat: add support for setting objectSelector on webhook

Signed-off-by: Cian Gallagher <[email protected]>

* feat: update objectSelector to match expressions

Signed-off-by: Cian Gallagher <[email protected]>

* chore: use out of the box label parser

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update app version

Signed-off-by: Cian Gallagher <[email protected]>

* fix: use parseSelector

Signed-off-by: Cian Gallagher <[email protected]>

* ci: update minikube action to latest release

Signed-off-by: Cian Gallagher <[email protected]>

* revert: undo ci changes. create seperate pr

Signed-off-by: Cian Gallagher <[email protected]>

* Trigger CI

Signed-off-by: Cian Gallagher <[email protected]>

* chore: update chart version & docs following previous merge

Signed-off-by: Cian Gallagher <[email protected]>

* docs: update docs

Signed-off-by: Cian Gallagher <[email protected]>

---------

Signed-off-by: Cian Gallagher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants