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

Restructure our API to make it work with client-gen #6661

Merged
merged 7 commits into from Oct 25, 2021
Merged

Restructure our API to make it work with client-gen #6661

merged 7 commits into from Oct 25, 2021

Conversation

rmohr
Copy link
Member

@rmohr rmohr commented Oct 21, 2021 •
edited

What this PR does / why we need it:

This PR prepares our codebase to allow synchronizing our API to https://github.com/kubevirt/api. In order to achive that the following preparations are done:

  • Move our api in api/v1 to apis/core/v1 which makes it live side-by-side to other kubevirt.io apigroups like apis/snapshot.
  • Change core/v1/register.go to be compatible with client-gen
  • Move custom GroupVersion registrations to kubecli.go

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

With this PR, other projects can already pull kubevirt/client-go and run something like the following on their codebase:

client-gen --clientset-name versioned \
    --input-base kubevirt.io/client-go/apis \
    --input core/v1 \
    --output-base ${PROJECT_DIR}/staging/src \
    --output-package ${CLIENT_GEN_BASE}/kubevirt/clientset \
    --go-header-file ${BOILERPLATE_DIR}/boilerplate.go.txt

# dependencies
client-gen --clientset-name versioned \
    --input-base kubevirt.io/containerized-data-importer/pkg/apis \
    --input core/v1beta1,upload/v1beta1 \
    --output-base ${PROJECT_DIR}/staging/src \
    --output-package ${CLIENT_GEN_BASE}/containerized-data-importer/clientset \
    --go-header-file ${BOILERPLATE_DIR}/boilerplate.go.txt

Release note:

Make the kubevirt api compatible with client-gen to make selecting compatible k8s golang dependencies easier

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 21, 2021
@kubevirt-bot kubevirt-bot added area/monitoring size/XXL kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/build-change Categorizes PRs as related to changing build files of virt-* components labels Oct 21, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 21, 2021

/cc @mhenriks
/cc @davidvossel
/cc @vladikr

@rmohr
Copy link
Member Author

rmohr commented Oct 21, 2021

/retest

1 similar comment
@rmohr
Copy link
Member Author

rmohr commented Oct 21, 2021

/retest

davidvossel
davidvossel reviewed Oct 21, 2021
View reviewed changes
Copy link
Member

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/approve

I can't practically look at every change here. In general I get what's happening though. Our core api moved to the apis directory along side snapshot and flavor apis. The core api is now generated in a way that makes it compatible with client-gen. Sounds great.

What's the plan for other projects that consume our API now. Will they be impacted by this change and need to adopt client-gen, or will their setup generally continue working? Given you made the backwards compatibility changes to kubecli, it seems like everything will continue to work as expected.

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021
@mhenriks
Copy link
Member

/retest

@mhenriks
Copy link
Member

looks good just not sure what's up with that test failure

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 25, 2021

looks good just not sure what's up with that test failure

Tests looked good after your retest.

@rmohr
Move the kubevirt api to core/api
ac41c25 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Adjust generate flows
f837a76 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
make generate
05ac412 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Remove client-go/api directory
2fcd1e5 
move OWNERS file to the new location.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Restructure core/v1 to be compatible with client-gen
a38afd4 
The kubecli client has special handling of v1alpha3 and v1. It allows
for instance using an env var to not register all versions. This logic
is move to kubecli.go and out of apis/core to avoid conflicts with
client-gen.

The final intent is that client-gen can focus on a clean v1 while
client-go still supports the whole set of workarounds which some other
projects which vendor client-go rely on.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Adjust api docs generator
be5435b 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Remove another unnecessary dependency from the API
e218786 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 25, 2021

Rebased due to merge conflict.

@rmohr
Copy link
Member Author

rmohr commented Oct 25, 2021

/retest

@mhenriks
Copy link
Member

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, mhenriks

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 [davidvossel,mhenriks]

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 ee4d9d8 into kubevirt:main Oct 25, 2021
@rmohr rmohr mentioned this pull request Oct 26, 2021
Merged
@tiraboschi tiraboschi mentioned this pull request Nov 2, 2021
Merged
11 tasks
tiraboschi added a commit to tiraboschi/hyperconverged-cluster-operator that referenced this pull request Nov 12, 2021
@tiraboschi
Bump KUBEVIRT to v0.47.1 and adapt the code
4ddf8d8 
Bump KUBEVIRT to v0.47.1 and adapt the code
for its new client-go struct introduced with
kubevirt/kubevirt#6661

v0.47.0_rc.0 and v0.47.0 never got accepted
because they were failing the upgrade tests,
trying now with v0.47.1.

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
kubevirt-bot pushed a commit to kubevirt/hyperconverged-cluster-operator that referenced this pull request Nov 12, 2021
@tiraboschi
Bump KUBEVIRT to v0.47.1 and adapt the code (#1582)
72d3317 
Bump KUBEVIRT to v0.47.1 and adapt the code
for its new client-go struct introduced with
kubevirt/kubevirt#6661

v0.47.0_rc.0 and v0.47.0 never got accepted
because they were failing the upgrade tests,
trying now with v0.47.1.

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
@victortoso victortoso mentioned this pull request Nov 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidvossel davidvossel davidvossel left review comments

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@rhrazdil rhrazdil Awaiting requested review from rhrazdil

@mhenriks mhenriks Awaiting requested review from mhenriks

@vladikr vladikr Awaiting requested review from vladikr

Assignees

@mhenriks mhenriks

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/monitoring dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/build-change Categorizes PRs as related to changing build files of virt-* components lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rmohr @mhenriks @kubevirt-bot @davidvossel