Skip to content

Comments

Remove kubevirt dependencies#574

Merged
kubevirt-bot merged 5 commits intok8snetworkplumbingwg:mainfrom
RamLavi:remove-kubevirt-dependencies
Nov 9, 2025
Merged

Remove kubevirt dependencies#574
kubevirt-bot merged 5 commits intok8snetworkplumbingwg:mainfrom
RamLavi:remove-kubevirt-dependencies

Conversation

@RamLavi
Copy link
Member

@RamLavi RamLavi commented Nov 6, 2025

What this PR does / why we need it:
This PR removes the dependency with kubevirt/go-client and makes the vendor much more easy to maintain.
the virtctl client is removed in favor of controller runtime client.

Special notes for your reviewer:

Release note:

NONE

@RamLavi
Copy link
Member Author

RamLavi commented Nov 7, 2025

/cherry-pick release-0.49

@kubevirt-bot
Copy link
Collaborator

@RamLavi: once the present PR merges, I will cherry-pick it on top of release-0.49 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-0.49

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Collaborator

@ormergi ormergi left a comment

Choose a reason for hiding this comment

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

Very nice cleanup

tests/tests.go Outdated
Comment on lines 96 to 97
_ = corev1.AddToScheme(scheme)
_ = v1.AddToScheme(scheme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems there is no need to register those (corev1, app/v1 clients), k8sClient should provide these clients, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

right. DONE

tests/tests.go Outdated
@@ -67,16 +71,42 @@ var (

type TestClient struct {
VirtClient kubecli.KubevirtClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider marking VirtClient as deprecated with message saying controller-runtime client should be used instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's going away in next commit by why not.
DONE

Comment on lines +278 to 280
err := testClient.CRClient.Get(context.Background(),
client.ObjectKey{Namespace: vm.Namespace, Name: vm.Name}, updatedVM)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Please consider in-lining the condition clause for reducing scope of returned values.
Here and in other occurrences.

Copy link
Member Author

Choose a reason for hiding this comment

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

let's to it in a follow up PR, this one has too much going on it as it is :)

var err error
virtualMachine, err = testClient.VirtClient.VirtualMachine(virtualMachine.Namespace).Get(context.TODO(),
virtualMachine.Name, metav1.GetOptions{})
err = testClient.CRClient.Get(context.TODO(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

context.TODO is deprecated, please use context.Background. Here and in other occurrences.

Dont we have linter rule for that? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Same - I'll do it in a follow up

Dont we have linter rule for that? 🤔

donno, but I'll see if the linter needs bumping..

)

type TestClient struct {
VirtClient kubecli.KubevirtClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah so this is removed eventually.. please consider mentioning in the first commit message virtClient is going to be removed in follow-up commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's written but maybe I should also explicitly state it as depreacted for formality

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nahh I dont think its necessary because its gone in the same PR, I already reviewed it and aware of the changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

already did :-p

@oshoval
Copy link
Member

oshoval commented Nov 9, 2025

please consider adding on PR desc that it was removed in favor of using controller runtime

Introduce kubernetes.Interface and controller-runtime client to
TestClient structure while keeping the existing VirtClient for backward
compatibility.

This allows gradual migration of test code without breaking existing
tests.

Signed-off-by: Ram Lavi <ralavi@redhat.com>
This covers all standard Kubernetes operations (Pods, Deployments,
Namespaces, ConfigMaps, Secrets, etc).

Signed-off-by: Ram Lavi <ralavi@redhat.com>
Replace all VirtualMachine CRUD operations from kubevirt client-go typed
client to controller-runtime client.

Signed-off-by: Ram Lavi <ralavi@redhat.com>
Remove VirtClient field and kubecli dependency from test infrastructure
since all operations now use kubernetes.Interface and controller-runtime
client.

Signed-off-by: Ram Lavi <ralavi@redhat.com>
Remove kubevirt.io/client-go from go.mod  All kubevirt API operations
now use controller-runtime client with kubevirt.io/api types.

This commit consists of the 'make vendor' changes.

Signed-off-by: Ram Lavi <ralavi@redhat.com>
@RamLavi RamLavi force-pushed the remove-kubevirt-dependencies branch from 6b5cf61 to d1e7bfa Compare November 9, 2025 14:34
@RamLavi
Copy link
Member Author

RamLavi commented Nov 9, 2025

Change: Address @ormergi 's review
PTAL

Copy link
Collaborator

@ormergi ormergi left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

@RamLavi
Copy link
Member Author

RamLavi commented Nov 9, 2025

/approve

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@kubevirt-bot kubevirt-bot merged commit fc28dd8 into k8snetworkplumbingwg:main Nov 9, 2025
5 checks passed
@kubevirt-bot
Copy link
Collaborator

@RamLavi: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}
Details

In response to this:

/cherry-pick release-0.49

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@oshoval
Copy link
Member

oshoval commented Nov 9, 2025

{"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}

funny one

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