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

feat: Webhook for CRDs #366

Merged
merged 22 commits into from
Jun 16, 2023
Merged

Conversation

Arvindthiru
Copy link
Contributor

@Arvindthiru Arvindthiru commented May 23, 2023

Description of your changes

Webhook for fleet CRDs

Created a fleet and joined a member cluster and used kusto query to identify users and their groups. Max size of groups for any user was 3.

image

All users were either part of system:authenticated or system:masters

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Added E2E

Special notes for your reviewer

@Arvindthiru Arvindthiru changed the title Webhook for CRDs feat: Webhook for CRDs May 26, 2023
@Arvindthiru Arvindthiru marked this pull request as ready for review May 26, 2023 00:36
@michaelawyu
Copy link
Contributor

Hi Arvind! I feel that there are a few general issues about this PR, which would be best if we resolve them first. I'll add them below; we can sync at a convenient time to go through them together if you'd like:

  • At this moment the webhook is validating CRD objects; but to cover the cases in the design doc, it should work on all objects
  • We should check in the webhook API object along with the code

@Arvindthiru Arvindthiru marked this pull request as draft June 2, 2023 19:31
@Arvindthiru Arvindthiru force-pushed the arvindth/fleetWebhook branch 2 times, most recently from 5adb4d6 to 8321d01 Compare June 13, 2023 20:33
@Arvindthiru Arvindthiru marked this pull request as ready for review June 13, 2023 20:43
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
@michaelawyu
Copy link
Contributor

Hi Arvind! I think everything looks good; for the remaining comments, if you have any concern, please let me know.

michaelawyu
michaelawyu previously approved these changes Jun 16, 2023
Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

LGTM ;) Thanks!

@ryanzhang-oss ryanzhang-oss merged commit 136e2d0 into Azure:main Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants