Introduce generated client for `core/v1` resources #11297

fossedihelm · 2024-02-19T16:27:55Z

What this PR does

Before this PR:
We are using the generated client for some resources, such as clone or migration, while we are using
the custom, manually maintained, for the core resources (VM, VMI...)

After this PR:
Convert using the generated client instead of the custom one.
This is the path will be followed:

Introduction of the generated client.
Redirect common api calls to the generated client
leaving the custom struct as extended wrapper.
Drop the custom wrapper structs, moving to the generated
resources only.

NB: This PR covers just the first two steps, since otherwise the changes will result
too big.*

Fixes #

Why we need it and why it was done in this way

The generated client has the advantage that the various methods will have the same standard signature. Currently, the method's signature between Get/Create/Update etc.. are different between VM/VMI/etc..:

sometimes a context is required, sometimes no
sometimes the options are required, sometimes no
sometimes a pointer is used as options, sometimes directly the struct

Also, alongside the generated client, the client-gen command generates the fakeclient that can be used in unit tests.
The fakeclient has the benefit, in unit tests, of simulating the real client and recording the changes, getting rid of mocking API requests (that can create misalignment to the reality) e.g. #10908

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

NONE

kubevirt-bot · 2024-02-19T16:27:58Z

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

fossedihelm · 2024-02-19T16:28:07Z

/test all

fossedihelm · 2024-02-19T16:54:01Z

/test pull-kubevirt-verify-go-mod

fossedihelm · 2024-02-19T17:02:24Z

/test all

aburdenthehand · 2024-02-21T15:32:34Z

/cc @alaypatel07

Barakmor1 · 2024-02-22T07:59:15Z

/cc

Barakmor1 · 2024-02-26T09:21:29Z

Hello @fossedihelm ,

I noticed in the commit message that you’ve outlined the task as consisting of three steps. In your commits, you’ve labeled them as “step 1” and “step 2.” However, I’m a bit confused because there seems to be a third step mentioned, but I only see two steps addressed in the commits. Could you please clarify if the second and third steps are included within the same commits, or am i missing something?

fossedihelm · 2024-02-26T09:28:33Z

Hello @fossedihelm ,

I noticed in the commit message that you’ve outlined the task as consisting of three steps. In your commits, you’ve labeled them as “step 1” and “step 2.” However, I’m a bit confused because there seems to be a third step mentioned, but I only see two steps addressed in the commits. Could you please clarify if the second and third steps are included within the same commits, or am i missing something?

Hey @Barakmor1 sorry about that, as I mentioned in the PR description, this PR only covers the first two steps (Now I have highlighted that part).
I thought having the full overview of the journey in the commits was good. Hope this helps. Thank you again!

Barakmor1 · 2024-02-26T10:08:36Z

Hello @fossedihelm ,
I noticed in the commit message that you’ve outlined the task as consisting of three steps. In your commits, you’ve labeled them as “step 1” and “step 2.” However, I’m a bit confused because there seems to be a third step mentioned, but I only see two steps addressed in the commits. Could you please clarify if the second and third steps are included within the same commits, or am i missing something?

Hey @Barakmor1 sorry about that, as I mentioned in the PR description, this PR only covers the first two steps (Now I have highlighted that part). I thought having the full overview of the journey in the commits was good. Hope this helps. Thank you again!

Thanks for clarifying

Barakmor1 · 2024-02-26T10:29:10Z

staging/src/kubevirt.io/client-go/kubecli/migration.go

-func (v *migration) UpdateStatus(vmi *v1.VirtualMachineInstanceMigration) (result *v1.VirtualMachineInstanceMigration, err error) {
+func (o *migration) UpdateStatus(vmi *v1.VirtualMachineInstanceMigration) (result *v1.VirtualMachineInstanceMigration, err error) {
 	result = &v1.VirtualMachineInstanceMigration{}
-	err = v.restClient.Put().
+	err = o.restClient.Put().
 		Name(vmi.ObjectMeta.Name).
-		Namespace(v.namespace).
-		Resource(v.resource).
+		Namespace(o.namespace).
+		Resource(o.resource).
 		SubResource("status").


I think you forgot to use o.VirtualMachineInstanceMigrationInterface.UpdateStatus here

Nice catch! You're right! Thanks for catching this! Done

Barakmor1 · 2024-02-26T10:31:10Z

I would also remove the 3 step from the commit message or at least mention that it will be handled in the future so it will be clear just from reading the commit messages.

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 1 of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm · 2024-02-26T10:57:44Z

I would also remove the 3 step from the commit message or at least mention that it will be handled in the future so it will be clear just from reading the commit messages.

Thank you! I dropped the mention of the 3rd step in the commits.

Barakmor1 · 2024-02-26T11:07:50Z

Thank you !
/lgtm

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the VMI resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm · 2024-03-07T15:38:43Z

Changes Addressed possible nil pointer reference.

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the VMIM resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the VMIRS resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the VM resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the KV resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Convert using the generated client instead of the custom one. This is the path will be followed: 1. Introduction of the generated client. 2. Redirect common api calls to the generated client leaving the custom struct as extended wrapper. This the step 2 for the VMIP resource of the conversion using the generated client. Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm · 2024-03-07T20:23:59Z

Changes:

Fixed dirty git tree ci failures
Convert PatchStatus implementation to Patch with subresources
@xpivarc PTAL Thanks

xpivarc · 2024-03-08T11:13:29Z

/hold cancel

fossedihelm · 2024-03-11T07:14:06Z

@Barakmor1 PTAL 🙂

Barakmor1 · 2024-03-11T07:36:56Z

/lgtm

kubevirt-commenter-bot · 2024-03-11T07:37:01Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

fossedihelm · 2024-04-23T13:13:39Z

/cherrypick release-1.2

kubevirt-bot · 2024-04-23T13:14:38Z

@fossedihelm: new pull request created: #11772

In response to this:

/cherrypick release-1.2

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/test-infra repository.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Feb 19, 2024

kubevirt-bot requested review from rthallisey and xpivarc February 19, 2024 16:28

fossedihelm force-pushed the generated_test_client branch from a6af07d to b52ea38 Compare February 19, 2024 16:53

fossedihelm marked this pull request as ready for review February 20, 2024 09:14

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2024

kubevirt-bot requested a review from vasiliy-ul February 20, 2024 09:14

enp0s3 mentioned this pull request Feb 20, 2024

go version go1.21.6 windows/amd64 Unable to create virtual machine using API #11112

Open

fossedihelm force-pushed the generated_test_client branch from b52ea38 to e6b727f Compare February 21, 2024 10:36

kubevirt-bot requested a review from alaypatel07 February 21, 2024 15:32

kubevirt-bot requested a review from Barakmor1 February 22, 2024 07:59

Barakmor1 reviewed Feb 26, 2024

View reviewed changes

fossedihelm force-pushed the generated_test_client branch from e6b727f to 928e9c4 Compare February 26, 2024 10:56

kubevirt-bot assigned Barakmor1 Feb 26, 2024

fossedihelm force-pushed the generated_test_client branch from 928e9c4 to 64f677c Compare March 7, 2024 15:21

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2024

fossedihelm force-pushed the generated_test_client branch from 64f677c to d0789bd Compare March 7, 2024 15:38

fossedihelm added 5 commits March 7, 2024 21:17

fossedihelm force-pushed the generated_test_client branch from d0789bd to 47832f1 Compare March 7, 2024 20:22

xpivarc mentioned this pull request Mar 8, 2024

Vm controller unit tests: fix return args #11322

Closed

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2024

kubevirt-bot merged commit f585c31 into kubevirt:main Mar 12, 2024
38 checks passed

This was referenced Mar 12, 2024

Generated client: drop custom wrapper methods of generated ones for vmi struct #11491

Closed

Generated client: drop custom wrapper methods of generated ones for vmi struct #11495

Merged

Generated client: embed generated methods into VM interface #11523

Merged

fossedihelm mentioned this pull request Apr 3, 2024

Generated client: embed generated methods into VMIM interface #11639

Merged

8 tasks

This was referenced Apr 11, 2024

Generated client: embed generated methods into KV interface #11695

Merged

Generated client: embed generated methods into VMIRS interface #11715

Merged

Generated client: embed generated methods into VMIP interface #11719

Merged

kubevirt-bot mentioned this pull request Apr 23, 2024

[release-1.2] Introduce generated client for core/v1 resources #11772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce generated client for `core/v1` resources #11297

Introduce generated client for `core/v1` resources #11297

fossedihelm commented Feb 19, 2024 •

edited

kubevirt-bot commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

aburdenthehand commented Feb 21, 2024

Barakmor1 commented Feb 22, 2024

Barakmor1 commented Feb 26, 2024 •

edited

fossedihelm commented Feb 26, 2024

Barakmor1 commented Feb 26, 2024

Barakmor1 Feb 26, 2024

fossedihelm Feb 26, 2024

Barakmor1 commented Feb 26, 2024

fossedihelm commented Feb 26, 2024

Barakmor1 commented Feb 26, 2024

fossedihelm commented Mar 7, 2024

fossedihelm commented Mar 7, 2024

xpivarc commented Mar 8, 2024

fossedihelm commented Mar 11, 2024

Barakmor1 commented Mar 11, 2024

kubevirt-commenter-bot commented Mar 11, 2024

fossedihelm commented Apr 23, 2024

kubevirt-bot commented Apr 23, 2024

Introduce generated client for core/v1 resources #11297

Introduce generated client for core/v1 resources #11297

Conversation

fossedihelm commented Feb 19, 2024 • edited

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

fossedihelm commented Feb 19, 2024

aburdenthehand commented Feb 21, 2024

Barakmor1 commented Feb 22, 2024

Barakmor1 commented Feb 26, 2024 • edited

fossedihelm commented Feb 26, 2024

Barakmor1 commented Feb 26, 2024

Barakmor1 Feb 26, 2024

Choose a reason for hiding this comment

fossedihelm Feb 26, 2024

Choose a reason for hiding this comment

Barakmor1 commented Feb 26, 2024

fossedihelm commented Feb 26, 2024

Barakmor1 commented Feb 26, 2024

fossedihelm commented Mar 7, 2024

fossedihelm commented Mar 7, 2024

xpivarc commented Mar 8, 2024

fossedihelm commented Mar 11, 2024

Barakmor1 commented Mar 11, 2024

kubevirt-commenter-bot commented Mar 11, 2024

fossedihelm commented Apr 23, 2024

kubevirt-bot commented Apr 23, 2024

Introduce generated client for `core/v1` resources #11297

Introduce generated client for `core/v1` resources #11297

fossedihelm commented Feb 19, 2024 •

edited

Barakmor1 commented Feb 26, 2024 •

edited