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.

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

Generated client: embed generated methods into VM interface #11523

Merged
merged 8 commits into from Apr 9, 2024
Merged

Generated client: embed generated methods into VM interface #11523

merged 8 commits into from Apr 9, 2024
+725 −724

Conversation

fossedihelm
Copy link
Contributor

What this PR does

Before this PR:
We have our custom virtualMachineInterface interface with the common Get, Delete, etc... methods and the customized ones (Migrate....)
From #11297 the common methods implementation works as a wrapper with different signatures for the generated ones, redirecting the requests to them.

After this PR:
Convert using the generated client signatures instead of the custom one. The generated virtualMachineInterface is embedded into our custom one, also providing new DeleteCollection and Watch methods.
The changes inherited are:

  • Patch and PatchStatus now use metav1.PatchOptions{} instead of *metav1.PatchOptions{}.
  • List now uses metav1.ListOptions{} instead of *metav1.ListOptions{}.
  • Get now uses metav1.GetOptions{} instead of *metav1.GetOptions{}.
  • Delete now uses metav1.DeleteOptions{} instead of *metav1.DeleteOptions{}.
  • Create now receives metav1.CreateOptions{} too.
  • Update and UpdateStatus now receive metav1.UpdateOptions{} too.
  • Added DeleteCollection, Watch methods.

Fixes #

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

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

For an easier review process the commits was splitted in the following manner:

  1. Align our custom common methods' signature to the generated one in the following order:
    • Create - introduced CreateOptions
    • Get - used GetOptions instead of the pointer
    • List - used ListOptions instead of the pointer
    • Update - introduced UpdateOptions
    • Delete - used DeleteOptions instead of the pointer
    • Patch and PatchStatus - used PatchOptions instead of the pointer
    • UpdateStatus - introduced UpdateOptions
  2. Embed the generated VirtualMachineInterface into the custom one.

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.

Release note

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Mar 15, 2024
@fossedihelm
Copy link
Contributor Author

@xpivarc @Barakmor1 Follow-up of #11495 for VirtualMachineInterface

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024
@kubevirt-bot kubevirt-bot 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 Mar 18, 2024
@kubevirt-bot kubevirt-bot 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 Mar 19, 2024
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2024
@fossedihelm fossedihelm changed the title Generated client: embed generated methods into VMI interface Generated client: embed generated methods into VM interface Mar 26, 2024
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024
@fossedihelm
Copy link
Contributor Author

/retest

@kubevirt-bot kubevirt-bot added the sig/buildsystem Denotes an issue or PR that relates to changes in the build system. label Apr 5, 2024
@fossedihelm
Copy link
Contributor Author

fossedihelm commented Apr 5, 2024 •
edited

@Barakmor1 Thanks! There was a build error for a new create/delete/get VM method introduced in metrics.go file https://github.com/kubevirt/kubevirt/compare/5d245a389283ef1c3bf4d3a310cacdccf90ace7c..d68e6cc65f2fbaab0dd82448d8e5136e8c8e3332#diff-5f9777f8195024e8eb5bc20dbc4c7336bdb99ac126777cb374157459a2dc81ce.
Addressed them in their specific commit with a rebase.
Thank you!

@Barakmor1
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024
@kubevirt-commenter-bot
Copy link

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
Copy link
Contributor Author

/retest

@fossedihelm
Copy link
Contributor Author

/hold
Wrong first commit

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2024
@fossedihelm
feat(client-gen): introduce CreateOptions for vm.Create method
994fb9e 
vm.Create method does not allow to pass the CreateOptions.
In order to align the custom method signature with the generated one
introduce the possibility to pass the CreateOptions.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): switch using GetOptions instead of pointer for vm.G…
d09983b 
…et method

vm.Get method allow to pass the pointer to GetOptions.
In order to align the custom method signature with the generated one
switch using the GetOptions instead of the pointer one.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): switch using ListOptions instead of pointer for vm.…
0688c9d 
…List method

vm.List method allow to pass the pointer to ListOptions.
In order to align the custom method signature with the generated one
switch using the ListOptions instead of the pointer one.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): introduce UpdateOptions for vm.Update method
e2ab960 
vm.Update method does not allow to pass the UpdateOptions.
In order to align the custom method signature with the generated one
introduce the possibility to pass the UpdateOptions.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): switch using DeleteOptions instead of pointer for v…
eabc578 
…m.Delete method

vm.Delete method allow to pass the pointer to DeleteOptions.
In order to align the custom method signature with the generated one
switch using the DeleteOptions instead of the pointer one.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): switch using PatchOptions instead of pointer for vm…
0bd7a96 
….Patch and vm.PatchStatus methods

vm.Patch and vm.PatchStatus method allow to pass the pointer to PatchOptions.
In order to align the custom method signature with the generated one
switch using the PatchOptions instead of the pointer one.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.
Signed-off-by: fossedihelm <ffossemo@redhat.com>

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): introduce UpdateOptions for vm.UpdateStatus method
3eba485 
vm.UpdateStatus method does not allow to pass the UpdateOptions.
In order to align the custom method signature with the generated one
introduce the possibility to pass the UpdateOptions.
This effort, once all the common methods' signatures are aligned with
the generated ones, will converge in the deletion of the
custom methods' implementations, embedding the generated interface.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
feat(client-gen): embed generated client for VM interface
5a42eb4 
The embed will provide us new methods (`DeleteCollection` and `Watch`).

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024
@fossedihelm
Copy link
Contributor Author

fossedihelm commented Apr 8, 2024 •
edited

/unhold
@Barakmor1 PTAL, there was a wrong first commit from another PR
Thanks @orelmisan for noticing it

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2024
@Barakmor1
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024
@kubevirt-commenter-bot
Copy link

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

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 9, 2024 •
edited

@fossedihelm: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-conformance-arm64 5a42eb4 link false /test pull-kubevirt-conformance-arm64

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. I understand the commands that are listed here.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit 92c5d36 into kubevirt:main Apr 9, 2024
38 of 39 checks passed
@fossedihelm fossedihelm mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@akalenyu akalenyu Awaiting requested review from akalenyu

@awels awels Awaiting requested review from awels

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@oshoval oshoval Awaiting requested review from oshoval

@RamLavi RamLavi Awaiting requested review from RamLavi

@rthallisey rthallisey Awaiting requested review from rthallisey

@vasiliy-ul vasiliy-ul Awaiting requested review from vasiliy-ul

Assignees

@Barakmor1 Barakmor1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/virtctl dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network sig/scale sig/storage size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fossedihelm @kubevirt-bot @Barakmor1 @kubevirt-commenter-bot @xpivarc