Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Transition CRDs from v1beta1 to v1 #1297

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Transition CRDs from v1beta1 to v1 #1297

merged 3 commits into from
Nov 17, 2020

Conversation

makkes
Copy link
Contributor

@makkes makkes commented Oct 12, 2020 •
edited
Loading

What this PR does / why we need it:
Transitions all the CRDs defined in the kubefed chart and sub-charts as well as those generated through kubefedctl from apiextensions.k8s.io/v1beta1 to v1.

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 #1296

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 12, 2020
@makkes makkes marked this pull request as draft October 12, 2020 14:48
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2020
@makkes makkes changed the title Transition chart CRDs from v1beta1 to v1 Transition CRDs from v1beta1 to v1 Oct 12, 2020
@makkes makkes force-pushed the apiextensions-v1 branch 4 times, most recently from c38423b to b73db7d Compare October 16, 2020 17:16
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2020
@makkes makkes force-pushed the apiextensions-v1 branch 4 times, most recently from 04be756 to 450bfca Compare October 23, 2020 20:50
@makkes makkes marked this pull request as ready for review October 23, 2020 20:50
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2020
@makkes makkes force-pushed the apiextensions-v1 branch 2 times, most recently from 2690b24 to db336df Compare October 26, 2020 12:50
@makkes
Copy link
Contributor Author

makkes commented Oct 26, 2020

@jimmidyson @hectorj2f @xunpan the CI checks are passing now (finally 🎉). Would you mind having a look at this PR, please?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2020
feat: transition to apiextensions.k8s.io/v1
2ab409a 
This commit transitions the CRDs from the kubefed chart and
sub-charts as well as all Go code generating CRDs.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2020
jimmidyson
jimmidyson reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

One question, but otherwise LGTM. Wonder if there is a nice way to also upgrade already deployed federated* CRDs to v1 in the same way when upgrading kubefed? Do we even need to worry about that?

pkg/kubefedctl/enable/validation.go Show resolved Hide resolved
@makkes
Copy link
Contributor Author

makkes commented Oct 30, 2020

Good point @jimmidyson. I will have a look into the upgrade behaviour. Do you know off the top of your head whether there's any upgrade testing in the e2e tests already?

@hectorj2f
Copy link
Contributor

@makkes I can confirm, there is not any upgrade testing.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@jimmidyson
Copy link
Contributor

jimmidyson commented Nov 16, 2020 •
edited
Loading

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2020
@makkes
Copy link
Contributor Author

makkes commented Nov 16, 2020

@jimmidyson: I was finally able to test the upgrade from a former version of kubefed:

  • checked out current master
  • ran make deploy.kind
  • attached another kind cluster
  • created a Deployment and federated it using kubefedctl
  • checked out this PR's branch and ran make deploy.kind again, updating the CRDs and controllers/webhooks
  • created a 2nd Deployment and federated it using the new kubefedctl binary

No interruption of the federated resources was observed and all new resources are federated properly.

@jimmidyson
Copy link
Contributor

Thanks @makkes for testing so thoroughly.

jimmidyson
jimmidyson reviewed Nov 16, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, makkes

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2020
@jimmidyson
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit ecd4900 into kubernetes-retired:master Nov 17, 2020
@makkes makkes deleted the apiextensions-v1 branch November 17, 2020 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hectorj2f hectorj2f hectorj2f approved these changes

@xunpan xunpan Awaiting requested review from xunpan

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@jimmidyson jimmidyson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transition from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1
4 participants
@makkes @hectorj2f @jimmidyson @k8s-ci-robot