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.

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

⚠️ Generate v1alpha4 types #3749

Merged
merged 6 commits into from
Oct 22, 2020

Conversation

srm09
Copy link
Contributor

@srm09 srm09 commented Oct 6, 2020

What this PR does / why we need it:

  • Generates types for v1alpha4
    • Core types
    • Experimental types
    • CAPBK types
    • KCP types
    • CAPD types
  • Remove v1alpha2 types
  • Updates imports to v1alpha4 packages

Which issue(s) this PR fixes:
Fixes #3428
Fixes #3800

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 6, 2020
@k8s-ci-robot k8s-ci-robot requested review from benmoss and ncdc October 6, 2020 16:44
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 6, 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 6, 2020
@srm09 srm09 marked this pull request as ready for review October 7, 2020 00:56
@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 7, 2020
@srm09
Copy link
Contributor Author

srm09 commented Oct 7, 2020

/test pull-cluster-api-verify

@srm09 srm09 force-pushed the v1alpha4-types branch 3 times, most recently from da37543 to ea5ef0d Compare October 7, 2020 05:59
@srm09
Copy link
Contributor Author

srm09 commented Oct 7, 2020

@vincepri what's the deal with the go-apidiff? How do I go about fixing it?

@srm09 srm09 force-pushed the v1alpha4-types branch 2 times, most recently from 65c6fbf to 807a363 Compare October 8, 2020 05:54
@srm09
Copy link
Contributor Author

srm09 commented Oct 8, 2020

@vincepri Running into a similar issue with auto generated conversion functions in controlplane/kubeadm/api/v1alpha3/zz_generated.conversion.go.

@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 8, 2020
@vincepri
Copy link
Member

vincepri commented Oct 9, 2020

@vincepri what's the deal with the go-apidiff? How do I go about fixing it?

That's normal that it's failing, it detects breaking changes from the previous commit, it's also an optional job

@vincepri Running into a similar issue with auto generated conversion functions in controlplane/kubeadm/api/v1alpha3/zz_generated.conversion.go.

Let's sync up today, @ncdc updated the related issue yesterday with more details. I can't think of any immediate workaround, unless we can overwrite the conversion functions that are referring cross package types

@ncdc
Copy link
Contributor

ncdc commented Oct 9, 2020

@srm09 @vincepri this is what I think we need to do to generate & use the right conversions:

  • Each API package needs to use a unique build tag
  • We need to invoke conversion-gen separately, once per API package
  • When we need to use conversion functions from packages outside the one we're generating, we have to specify those packages as --extra-peer-dirs using the fully qualified package name

All of these changes can be made in the Makefile.

Here's a start to the conversion-gen calls that we'll need

  • 1 for ./api/v1alpha3, build-tag = something like ignore_autogenerated_core_v1alpha3
  • 1 for ./bootstrap/kubeadm/api/v1alpha3, extra-peer-dirs = sigs.k8s.io/cluster-api/api/v1alpha3, build-tag ~ ignore_autogenerated_kubeadm_bootstrap_v1alpha3
  • etc

Basically start with the foundation and then build up, adding extra-peer-dirs as needed.

@srm09 srm09 force-pushed the v1alpha4-types branch 2 times, most recently from bd12032 to 0877908 Compare October 10, 2020 00:46
@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 10, 2020
@srm09
Copy link
Contributor Author

srm09 commented Oct 11, 2020

@ncdc The changes that you suggested above to add build tags and extra-peer-dirs to the makefile targets for code generation. I added the bare minimums for this PR, and I can create a separate issue and PR entirely dedicated to all the changes you mentioned above.

@srm09
Copy link
Contributor Author

srm09 commented Oct 19, 2020

@ncdc Is this waiting on any more changes to be merged to the default branch?

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/assign @ncdc
for final lgtm

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

ncdc commented Oct 20, 2020

@srm09 let me give it another 👀 before merging.

@vincepri
Copy link
Member

/approve cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2020
@srm09
Copy link
Contributor Author

srm09 commented Oct 21, 2020

New changes are detected. LGTM label has been removed.

Had to fix tests after rebase

@vincepri
Copy link
Member

/test pull-cluster-api-test-main

Copy link
Contributor

@ncdc ncdc left a comment

Choose a reason for hiding this comment

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

@srm09 thanks for your hard work on this! A few questions:

  • Do we need to update cmd/clusterctl/... in this PR (it still imports v1alpha3 and the CRDs it owns are still v1alpha3)?
  • test/framework/clusterctl/... still has v1alpha3 imports?
  • Some of the files in test/infrastructure/docker/{examples,templates} still use v1alpha3?
  • controllers/cluster_controller.go imports sigs.k8s.io/cluster-api/exp/api/v1alpha3 - this probably needs to be change to v1alpha4 and let's update the import alias to expv1?
  • The following still have sample/test data using v1alpha3 (may be ok, though?)
    • util/util_test.go
    • util/yaml/yaml_test.go
    • controlplane/kubeadm/internal/machinefilters/machine_filters_test.go
    • controlplane/kubeadm/controllers/helpers_test.go
    • exp/controllers/machinepool_controller_phases_test.go
    • exp/controllers/machinepool_controller_test.go
    • controllers/machine_controller_phases_test.go
    • controllers/mdutil/util_test.go
    • controllers/machine_controller_test.go

@srm09
Copy link
Contributor Author

srm09 commented Oct 21, 2020

@ncdc

Do we need to update cmd/clusterctl/... in this PR (it still imports v1alpha3 and the CRDs it owns are still v1alpha3)?

The issue did not list the creation of the clusterctl types, hence did not include those in this PR. I can create a new issue to address that.

test/framework/clusterctl/... still has v1alpha3 imports?

same as above

The following still have sample/test data using v1alpha3 (may be ok, though?)
util/util_test.go
util/yaml/yaml_test.go
controlplane/kubeadm/internal/machinefilters/machine_filters_test.go
controlplane/kubeadm/controllers/helpers_test.go
exp/controllers/machinepool_controller_phases_test.go
exp/controllers/machinepool_controller_test.go
controllers/machine_controller_phases_test.go
controllers/mdutil/util_test.go
controllers/machine_controller_test.go

The test data just points to the API version, not sure if it is really necessary to change it. But if we wanna get rid of the v1alpha3 references and keep a single version number in tests around, I can update those.

UPDATE: Replaced the changes in the test files to use v1alpha4.

srm09 and others added 6 commits October 21, 2020 16:05
- Updates config/CRDs with the new generated types
- Updates the config/webhook for validations of v1alpha4 types
- Set the kubebuilder:storageversion for the v1alpha4 types and reset
the one for v1alpha3 types
- Adds v1alpha4 as the conversion hub and v1alpha3 as the spoke
- Removes the webhooks for v1alpha3 types
- Updates the PROJECT file

Signed-off-by: Sagar Muchhal <[email protected]>
- Sets the storage version only for the v1alpha4 types
- Regenerates the manifests under the config directory
- Marks v1alpha4 as the conversion hub
- Removes webhooks for v1alpha3 types
- Updates the types in PROJECT file

Signed-off-by: Sagar Muchhal <[email protected]>
- Sets the storage version only for the v1alpha4 types
- Regenerates the manifests under the config directory
- Marks the v1alpha4 version as conversion hub
- Removes webhooks for v1alpha3 types
- Updates the types in PROJECT file

Signed-off-by: Sagar Muchhal <[email protected]>
- Sets the storage version only for the v1alpha4 types
- Generates the conversion webhook for v1alpha3 -> v1alpha4
- Generates the v1alpha4 equivalents for exp API types
- Regenerates the manifests under the config directory
- Removes webhooks for v1alpha3 types
- Updates the types in PROJECT file

Signed-off-by: Sagar Muchhal <[email protected]>
- Generates v1alpha4 types for exp and exp/addons dir
- Updates manifests and webhook configs
- Updates PROJECT files to include alpha4 types
- Updates Makefile to generate conversion files
- Removes webhooks for v1alpha3 types

Signed-off-by: Sagar Muchhal <[email protected]>
- Replace v1alpha3 imports with v1alpha4 ones
- Removes v1alpha2 types
  Updates PROJECT files for core and bootstrap
  Update the manifests to remove v1alpha2 types
  Remove refs of v1alpha2 from Makefile
- Removes v1alpha3 webhooks from manager
- Updates code-gen Makefile targets
  Before running the conversion-gen, we delete the existing generated
  files to ensure clean generation of API conversion code.

Signed-off-by: Sagar Muchhal <[email protected]>
Co-authored-by: Vince Prignano <[email protected]>
@ncdc
Copy link
Contributor

ncdc commented Oct 22, 2020

/lgtm

🚀 🎉 🎂

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 22, 2020

@srm09: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-apidiff-main e73e3d9 link /test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@ncdc
Copy link
Contributor

ncdc commented Oct 22, 2020

/test pull-cluster-api-test-main

@k8s-ci-robot k8s-ci-robot merged commit 2dd66d6 into kubernetes-sigs:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 "Looks good to me", 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.

Delete conversion files before running conversion-gen in Makefiles Generate v1alpha4 types
4 participants