search

kubernetes-sigs / cluster-api-provider-kubevirt

Cluster API Provider for KubeVirt
Apache License 2.0
110 stars 63 forks source link

Bump dependencies #267

Closed nunnatsa closed 11 months ago

nunnatsa commented 11 months ago

The primary goal of this PR is to bump the dependencies. In attempting to bump the dependences @nunnatsa discovered a conflict between kubevirt/client-go and updating the core k8s depenedences. Upon further inspection, it was determined that we only needed the kubevirt/client-go package for our e2e tests, and not the capk controllers themselfs.

So, in order to update the core k8s dependences for the capk controllers without entering into dependency hell, Nahshon separated the dependences for the e2e tests and capk controllers. This removed us from having to have kubevirt/client-go in our primary go.mod file.

In all, his changes are...

Bump dependencies and separate e2e test and controller dependencies
k8s-ci-robot commented 11 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/blob/main/OWNERS)~~ [nunnatsa] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
nunnatsa commented 11 months ago

/ok-to-test

coveralls commented 11 months ago

Pull Request Test Coverage Report for Build 6584385022

Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/kubevirtmachine_controller.go 5 16 31.25%
controllers/kubevirtcluster_controller.go 1 15 6.67%
pkg/webhookhandler/validator.go 26 49 53.06%
<!-- Total: 35 83 42.17% -->
Files with Coverage Reduction New Missed Lines %
controllers/kubevirtcluster_controller.go 1 45.7%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 6232901575: 12.3%
Covered Lines: 946
Relevant Lines: 1526
💛 - Coveralls
agradouski commented 11 months ago

this PR does a bit more than just bumping dependencies. perhaps it's worth splitting it to several focused PRs @nunnatsa, to simplify the review? or, please add more comments about various updates you're making to the logic. thanks

nunnatsa commented 11 months ago

this PR does a bit more than just bumping dependencies. perhaps it's worth splitting it to several focused PRs @nunnatsa, to simplify the review? or, please add more comments about various updates you're making to the logic. thanks

@agradouski - there is no logic change. All the code changes are required by API changes in the bumped modules. I don't know how to split it to smaller commits, if I want any commit to be compile-able.

davidvossel commented 11 months ago

this PR does a bit more than just bumping dependencies. perhaps it's worth splitting it to several focused PRs @nunnatsa, to simplify the review? or, please add more comments about various updates you're making to the logic. thanks @agradouski - there is no logic change. All the code changes are required by API changes in the bumped modules. I don't know how to split it to smaller commits, if I want any commit to be compile-able.

Yeah, there are a few things going on here. Here's some further context.

The primary goal of this PR is to bump the dependencies. In attempting to bump the dependences @nunnatsa discovered a conflict between kubevirt/client-go and updating the core k8s depenedences. Upon further inspection, it was determined that we only needed the kubevirt/client-go package for our e2e tests, and not the capk controllers themselfs.

So, in order to update the core k8s dependences for the capk controllers without entering into dependency hell, Nahshon separated the dependences for the e2e tests and capk controllers. This removed us from having to have kubevirt/client-go in our primary go.mod file.

In all, his changes are...

  1. Separate dependencies for the controllers and e2e into separate go.mod files
  2. Update the code to work with the new dependencies. (that meant moving the validation webhook logic and passing around the context object)
  3. Removing Passt support (the e2e test is failing and we don't intend to support passt binding anymore)