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

⚠️ Updates controller-runtime version to v0.8.1 #4109

Merged

Conversation

srm09
Copy link
Contributor

@srm09 srm09 commented Jan 22, 2021

What this PR does / why we need it:
This patch updates the controller-runtime version to v0.8.1. The k8s dependencies in the codebase have been updated to v0.20.2 since this version of controller-runtime has those dependencies.

Which issue(s) this PR fixes:
Fixes #4096

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 22, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 22, 2021
@srm09 srm09 force-pushed the update-controller-runtime-version branch from 21eff81 to 1ab40d2 Compare January 22, 2021 18:26
@vincepri
Copy link
Member

This is a ⚠️ because of the changes needed in the types

@srm09
Copy link
Contributor Author

srm09 commented Jan 22, 2021

/retitle ⚠️ Updates controller-runtime version to v0.8.1

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Updates controller-runtime version to v0.8.1 ⚠️ Updates controller-runtime version to v0.8.1 Jan 22, 2021
@srm09
Copy link
Contributor Author

srm09 commented Jan 22, 2021

@vincepri Any suggestions?

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.

/lgtm
/hold

@CecileRobertMichon @detiber FYI please review, there are required changes to the slice types to use non-pointer structs

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2021
@sedefsavas
Copy link

Shall we add this requirement to Cluster API v1alpha3 compared to v1alpha4 doc as well?

This involved bumping the code-gen version to 0.20 because the signature
of the function had been updated. Required regenerating the conversion
code.
Also updated the types containing slice of pointers of custom object
types to slice of custom objects to avoid conversion errors.
Update the v1a3 -> v1a4 upgrade docs to record this requirement.

Signed-off-by: Sagar Muchhal <[email protected]>
@srm09 srm09 force-pushed the update-controller-runtime-version branch from 1ab40d2 to 1356c4a Compare January 25, 2021 17:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2021
@vincepri
Copy link
Member

/test pull-cluster-api-test-main

@srm09
Copy link
Contributor Author

srm09 commented Jan 27, 2021

@CecileRobertMichon @detiber Did y'all get a chance to take a look at this one?

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 28, 2021
@srm09
Copy link
Contributor Author

srm09 commented Jan 29, 2021

/hold cancel

@detiber
Copy link
Member

detiber commented Jan 29, 2021

@CecileRobertMichon @detiber FYI please review, there are required changes to the slice types to use non-pointer structs

The changes to the slice types seem like they are a concerning issue since it seems to effect not just the v1alpha4 types but also the v1alpha3 types. It seems like this is a bug that we need to get addressed in the code generator.

@vincepri
Copy link
Member

@detiber Are you thinking about folks importing and using v1alpha3 type while using a v0.4.x version?

@detiber
Copy link
Member

detiber commented Jan 29, 2021

@detiber Are you thinking about folks importing and using v1alpha3 type while using a v0.4.x version?

No, my concern was around how it would potentially affect existing resources that are already defined, would this break us being able to deserialize previously stored resources in etcd? I don't see any changes to the CRD openapi definition, so maybe that is the case.

If I'm reading the apiextensions-apiserver registry code right, it looks like the resource is stored in etcd as Unstructured, so I suppose as long as there isn't any impact on serialization between yaml/json and golang structs or between unstructured.Unstructured and the golang structs then it's likely a non-issue.

After digging through things, it looks like it's a non-issue, the type change to the v1alpha3 resource caught me a bit off guard :)

@detiber
Copy link
Member

detiber commented Feb 1, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber

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 Feb 1, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0d4e430 into kubernetes-sigs:master Feb 1, 2021
@srm09 srm09 deleted the update-controller-runtime-version branch February 1, 2021 20:29
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade the controller-runtime version to v0.8.x
6 participants