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: Add Conversion Webhooks for V1 APIs #1356

Merged

Conversation

engedaam
Copy link
Contributor

@engedaam engedaam commented Jun 26, 2024

Fixes #N/A

Description

  • Add Conversion webhook APIs for NodePool and NodeClaims

How was this change tested?

  • make presubmit

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 26, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 26, 2024
@engedaam engedaam marked this pull request as ready for review June 26, 2024 22:13
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2024
@k8s-ci-robot k8s-ci-robot requested a review from jmdeal June 26, 2024 22:13
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9687450759

Details

  • 271 of 338 (80.18%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 78.962%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodepool_conversion.go 120 130 92.31%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/zz_generated.deepcopy.go 16 33 48.48%
pkg/apis/v1/nodeclaim_conversion.go 117 135 86.67%
Totals Coverage Status
Change from base Build 9687383593: 0.7%
Covered Lines: 8903
Relevant Lines: 11275

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024

Pull Request Test Coverage Report for Build 9687470480

Details

  • 271 of 338 (80.18%) changed or added relevant lines in 9 files are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.7%) to 78.971%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodepool_conversion.go 120 130 92.31%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/zz_generated.deepcopy.go 16 33 48.48%
pkg/apis/v1/nodeclaim_conversion.go 117 135 86.67%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/disruption/drift.go 2 89.09%
pkg/scheduling/requirements.go 2 98.01%
pkg/controllers/node/termination/terminator/eviction.go 2 89.09%
Totals Coverage Status
Change from base Build 9687383593: 0.7%
Covered Lines: 8904
Relevant Lines: 11275

💛 - Coveralls

kwok/charts/crds/karpenter.sh_nodeclaims.yaml Outdated Show resolved Hide resolved
kwok/charts/crds/karpenter.sh_nodeclaims.yaml Outdated Show resolved Hide resolved
kwok/charts/crds/karpenter.sh_nodepools.yaml Outdated Show resolved Hide resolved
kwok/main.go Show resolved Hide resolved
pkg/apis/crds/karpenter.sh_nodeclaims.yaml Outdated Show resolved Hide resolved
pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
pkg/operator/operator.go Outdated Show resolved Hide resolved
pkg/webhooks/webhooks.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 28, 2024
@engedaam engedaam force-pushed the implement-conversion-webhook branch 3 times, most recently from befd55c to d0869ae Compare July 1, 2024 14:54
@engedaam engedaam force-pushed the implement-conversion-webhook branch from d0869ae to e7631e6 Compare July 1, 2024 14:58
@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9746470447

Details

  • 242 of 272 (88.97%) changed or added relevant lines in 7 files are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.3%) to 79.123%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodeclaim_conversion.go 112 120 93.33%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/test/expectations/expectations.go 2 93.69%
pkg/controllers/node/termination/controller.go 2 59.81%
pkg/webhooks/webhooks.go 9 0.0%
Totals Coverage Status
Change from base Build 9721804824: 0.3%
Covered Lines: 8842
Relevant Lines: 11175

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9746484102

Details

  • 242 of 272 (88.97%) changed or added relevant lines in 7 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 79.078%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodeclaim_conversion.go 112 120 93.33%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/node/termination/controller.go 2 59.81%
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
Totals Coverage Status
Change from base Build 9721804824: 0.3%
Covered Lines: 8837
Relevant Lines: 11175

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9746527988

Details

  • 242 of 272 (88.97%) changed or added relevant lines in 7 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 79.123%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodeclaim_conversion.go 112 120 93.33%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/test/expectations/expectations.go 2 93.69%
pkg/controllers/node/termination/controller.go 2 59.81%
Totals Coverage Status
Change from base Build 9721804824: 0.3%
Covered Lines: 8842
Relevant Lines: 11175

💛 - Coveralls

pkg/webhooks/webhooks.go Outdated Show resolved Hide resolved
pkg/webhooks/webhooks.go Outdated Show resolved Hide resolved
pkg/webhooks/webhooks.go Outdated Show resolved Hide resolved
pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Outdated Show resolved Hide resolved
pkg/operator/operator.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion_test.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9782771807

Details

  • 242 of 272 (88.97%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 79.092%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 14 18 77.78%
pkg/operator/operator.go 0 4 0.0%
pkg/apis/v1/nodeclaim_conversion.go 112 120 93.33%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/test/expectations/expectations.go 2 93.69%
Totals Coverage Status
Change from base Build 9772878439: 0.3%
Covered Lines: 8852
Relevant Lines: 11192

💛 - Coveralls

kwok/main.go Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/nodeclaim_conversion.go Show resolved Hide resolved
pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the implement-conversion-webhook branch from 8c4be43 to 66f7f21 Compare July 3, 2024 21:07
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9784730774

Details

  • 195 of 225 (86.67%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 78.959%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 9 77.78%
pkg/operator/operator.go 0 3 0.0%
pkg/apis/v1/nodepool_conversion.go 109 118 92.37%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/node/termination/terminator/eviction.go 2 89.09%
Totals Coverage Status
Change from base Build 9772878439: 0.2%
Covered Lines: 8800
Relevant Lines: 11145

💛 - Coveralls

@engedaam engedaam force-pushed the implement-conversion-webhook branch from 66f7f21 to 08f7133 Compare July 3, 2024 21:15
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9784823970

Details

  • 195 of 225 (86.67%) changed or added relevant lines in 7 files are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 78.923%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 9 77.78%
pkg/operator/operator.go 0 3 0.0%
pkg/apis/v1/nodepool_conversion.go 109 118 92.37%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/test/expectations/expectations.go 2 93.69%
pkg/controllers/node/termination/terminator/eviction.go 2 89.09%
pkg/webhooks/webhooks.go 9 0.0%
Totals Coverage Status
Change from base Build 9772878439: 0.2%
Covered Lines: 8796
Relevant Lines: 11145

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9784920257

Details

  • 195 of 225 (86.67%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 78.941%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 9 77.78%
pkg/operator/operator.go 0 3 0.0%
pkg/apis/v1/nodepool_conversion.go 109 118 92.37%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/disruption/drift.go 2 89.09%
Totals Coverage Status
Change from base Build 9772878439: 0.2%
Covered Lines: 8798
Relevant Lines: 11145

💛 - Coveralls

pkg/operator/injection/injection.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Show resolved Hide resolved
pkg/apis/v1/nodeclaim_conversion_test.go Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion_test.go Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion_test.go Outdated Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion_test.go Show resolved Hide resolved
pkg/apis/v1/nodepool_conversion.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the implement-conversion-webhook branch 3 times, most recently from 44f4088 to 9f3f1fc Compare July 3, 2024 22:43
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9785588418

Details

  • 193 of 224 (86.16%) changed or added relevant lines in 7 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 78.885%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 10 70.0%
pkg/operator/operator.go 0 3 0.0%
pkg/apis/v1/nodepool_conversion.go 107 116 92.24%
pkg/webhooks/webhooks.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
Totals Coverage Status
Change from base Build 9772878439: 0.1%
Covered Lines: 8791
Relevant Lines: 11144

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9785718428

Details

  • 187 of 220 (85.0%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 78.923%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 10 70.0%
pkg/operator/operator.go 0 3 0.0%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/nodepool_conversion.go 101 112 90.18%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/disruption/drift.go 2 89.09%
Totals Coverage Status
Change from base Build 9772878439: 0.2%
Covered Lines: 8792
Relevant Lines: 11140

💛 - Coveralls

@engedaam engedaam force-pushed the implement-conversion-webhook branch 3 times, most recently from f8e0b90 to 8eb8b70 Compare July 3, 2024 23:35
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9786007465

Details

  • 187 of 220 (85.0%) changed or added relevant lines in 7 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.1%) to 78.86%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 10 70.0%
pkg/operator/operator.go 0 3 0.0%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/nodepool_conversion.go 101 112 90.18%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/disruption/drift.go 2 89.09%
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
Totals Coverage Status
Change from base Build 9772878439: 0.1%
Covered Lines: 8785
Relevant Lines: 11140

💛 - Coveralls

@engedaam engedaam force-pushed the implement-conversion-webhook branch from 8eb8b70 to be9c580 Compare July 3, 2024 23:39
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 3, 2024
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9786124945

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 189 of 222 (85.14%) changed or added relevant lines in 7 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.1%) to 78.864%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 10 70.0%
pkg/operator/operator.go 0 3 0.0%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/nodepool_conversion.go 103 114 90.35%
Files with Coverage Reduction New Missed Lines %
pkg/test/expectations/expectations.go 2 93.69%
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
Totals Coverage Status
Change from base Build 9772878439: 0.1%
Covered Lines: 8787
Relevant Lines: 11142

💛 - Coveralls

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 4, 2024
@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9786712933

Details

  • 189 of 222 (85.14%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 78.824%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/v1/nodeclaim_conversion.go 79 81 97.53%
pkg/apis/v1beta1/nodeclaim_conversion.go 0 2 0.0%
pkg/apis/v1beta1/nodepool_conversion.go 0 2 0.0%
pkg/operator/injection/injection.go 7 10 70.0%
pkg/operator/operator.go 0 3 0.0%
pkg/webhooks/webhooks.go 0 10 0.0%
pkg/apis/v1/nodepool_conversion.go 103 114 90.35%
Files with Coverage Reduction New Missed Lines %
pkg/scheduling/requirements.go 2 98.01%
Totals Coverage Status
Change from base Build 9786565889: 0.2%
Covered Lines: 8863
Relevant Lines: 11244

💛 - Coveralls

Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: engedaam, njtran

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 merged commit e356a51 into kubernetes-sigs:main Jul 5, 2024
13 checks passed
@engedaam engedaam deleted the implement-conversion-webhook branch August 26, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants