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

Migrate to v1 APIs for CRD, VWH, MWH #1434

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

yiqigao217
Copy link
Contributor

@yiqigao217 yiqigao217 commented Mar 12, 2021

Part of #1371

Migrate apiextensions from v1beta1 to v1:

  1. Upgrade controller-tools to generate kubebuilder markers to use v1
    apis;
  2. Replace v1beta1 with v1 in yaml files and patches;
  3. Fix schema changes from v1beta1 to v1:
    a) SideEffects is required for v1 MWH, VWH;
    b) AdmissionReviewVersions is mandatory for v1;
    c) webhookClientConfig schema change in CRD;
    d) conversionReviewVersions in v1 CRD;
    e) add 1 more level of indention in enum singleton patch;
    f) add spec.scope and schema in testing CRD (they are required in v1);
    g) add v1beta1 to CRD spec.conversion.conversionReviewVersions and
    VWH admissionReviewVersions, since it's required in K8s 1.16 cluster.
  4. E2e test in 1.16 and 1.17 K8s cluster and make test(got below issue
    and fixed);
  5. Reinstall latest kubebuilder since controller-runtime envtest uses
    etcd and kube-apiserver loaded by default from
    /usr/local/kubebuilder/bin. (TODO will document this in the next PR in
    make test and HNC developer README).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2021
@k8s-ci-robot k8s-ci-robot requested review from Fei-Guo and rjbez17 March 12, 2021 16:43
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 12, 2021
@yiqigao217
Copy link
Contributor Author

/assign @adrianludwin
/assign @rjbez17

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

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold
/assign @rjbez17

Looks great but I wouldn't mind another set of eyes on this

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, yiqigao217

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 Mar 12, 2021
@yiqigao217
Copy link
Contributor Author

/hold

Seems v1beta1 is required in the CRD spec.conversion.conversionReviewVersions and VWH admissionReviewVersions in K8s 1.16 cluster. Testing now.

Migrate apiextensions from v1beta1 to v1:
1. Upgrade controller-tools to generate kubebuilder markers to use v1
apis;
2. Replace v1beta1 with v1 in yaml files and patches;
3. Fix schema changes from v1beta1 to v1:
 a) SideEffects is required for v1 MWH, VWH;
 b) AdmissionReviewVersions is mandatory for v1;
 c) webhookClientConfig schema change in CRD;
 d) conversionReviewVersions in v1 CRD;
 e) add 1 more level of indention in enum singleton patch;
 f) add spec.scope and schema in testing CRD (they are required in v1);
 g) add `v1beta1` to CRD `spec.conversion.conversionReviewVersions` and
 VWH `admissionReviewVersions`, since it's required in K8s 1.16 cluster.
4. E2e test in 1.16 and 1.17 K8s cluster and `make test`(got below issue
and fixed);
5. Reinstall latest kubebuilder since controller-runtime envtest uses
etcd and kube-apiserver loaded by default from
/usr/local/kubebuilder/bin. (TODO will document this in the next PR in
 `make test` and HNC developer README).
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2021
@yiqigao217
Copy link
Contributor Author

Seems v1beta1 is required in the CRD spec.conversion.conversionReviewVersions and VWH admissionReviewVersions in K8s 1.16 cluster. Testing now.

Tested on 1.16 cluster after adding v1beta1 to the list and it worked. Also retested it in 1.17 cluster and it worked. Updated the PR message to reflect it.

@rjbez17
Copy link

rjbez17 commented Mar 13, 2021

/lgtm

Did you need the hold still for some testing? Sounds like it's all good but I'll leave it just in case.

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2021
@adrianludwin
Copy link
Contributor

lgtm as well - @yiqigao217 please cancel the hold when you're comfortable.

@yiqigao217
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8626a0b into kubernetes-retired:master Mar 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

4 participants