Skip to content

Move webhook code to internal/webhook #727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 7, 2024
Merged

Move webhook code to internal/webhook #727

merged 1 commit into from
Feb 7, 2024

Conversation

qbarrand
Copy link
Contributor

@qbarrand qbarrand commented Feb 6, 2024

Remove webhook code from the api{,-hub} packages to avoid depending on controller-runtime there.
Implement the new admission.CustomValidator interface as the old webhook.Validator is now deprecated.
Bump controller-gen to v0.14.0.

/cc @mresvanis @yevgeny-shnaidman

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2024
@netlify Netlify
Copy link

netlify bot commented Feb 6, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name Link
🔨 Latest commit ca8fe8f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/65c38fd934f9c500087d60e1
😎 Deploy Preview https://deploy-preview-727--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qbarrand

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024 •
edited
Loading

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (fa23a9b) 79.09% compared to head (1b60a35) 78.40%.
Report is 5 commits behind head on main.

Files Patch % Lines
internal/controllers/module_nmc_reconciler.go 73.91% 8 Missing and 4 partials ⚠️
internal/webhook/module.go 70.96% 6 Missing and 3 partials ⚠️
internal/webhook/namespace.go 61.53% 5 Missing ⚠️
internal/meta/labels.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
- Coverage   79.09%   78.40%   -0.70%     
==========================================
  Files          51       49       -2     
  Lines        5109     3959    -1150     
==========================================
- Hits         4041     3104     -937     
+ Misses        882      663     -219     
- Partials      186      192       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yevgeny-shnaidman
yevgeny-shnaidman reviewed Feb 7, 2024
View reviewed changes
internal/webhook/hub/managedclustermodule_webhook.go Outdated
m admission.CustomValidator
}

func NewManagedClusterModule(logger logr.Logger) *ManagedClusterModuleValidator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets change the New... function name also to NewManagedClusterModuleValidator

internal/webhook/module.go Outdated
logger logr.Logger
}

func NewModule(logger logr.Logger) *ModuleValidator {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets change the New function to NewModuleValidator

@qbarrand
Move webhook code to internal/webhook
ca8fe8f 
Remove webhook code from the api{,-hub} packages to avoid depending on
controller-runtime there.
Implement the new admission.CustomValidator interface as the old
webhook.Validator is now deprecated.
Bump controller-gen to v0.14.0.
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2024
@k8s-ci-robot k8s-ci-robot merged commit e499d6f into kubernetes-sigs:main Feb 7, 2024
@qbarrand qbarrand deleted the move-webhooks branch February 7, 2024 14:41
@qbarrand qbarrand mentioned this pull request Feb 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yevgeny-shnaidman yevgeny-shnaidman yevgeny-shnaidman left review comments

@mresvanis mresvanis Awaiting requested review from mresvanis

Assignees

@yevgeny-shnaidman yevgeny-shnaidman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qbarrand @k8s-ci-robot @codecov-commenter @yevgeny-shnaidman