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

Upgrade controller-runtime and k8s packages #282

Closed
wants to merge 1 commit into from
Closed

Upgrade controller-runtime and k8s packages #282

wants to merge 1 commit into from

Conversation

xiaoxubeii
Copy link

@xiaoxubeii xiaoxubeii commented Jun 15, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

#280 needs webhhook for delete verb, but it is broken in controller-runtime v0.11.1. Fixed PR is kubernetes-sigs/controller-runtime#1278.

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 15, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @xiaoxubeii. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 15, 2022
@denkensk
Copy link
Member

If we bump to 1.24 and merge #280, does it mean our project can only be available in v1.24.
This is not good, because most of the clusters have not been upgraded to 1.24.

@xiaoxubeii
Copy link
Author

If we bump to 1.24 and merge #280, does it mean our project can only be available in v1.24. This is not good, because most of the clusters have not been upgraded to 1.24.

I think Kubernetes should be backwards compatible and update references doesn't mean only this version and above are supported.

@denkensk
Copy link
Member

I think Kubernetes should be backwards compatible and update references doesn't mean only this version and above are supported.

I am not sure. But there are always many compatibility issues in different places. Can you provide some compatibility tests in 1.23 with this PR(bump to 1.24)？

@denkensk
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 15, 2022
ArangoGutierrez
ArangoGutierrez suggested changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment •
edited

Choose a reason for hiding this comment

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

Could you edit PR title and commit to reflect what are we bumping, thanks

"Bump to v0.24" is not very verbose to keep as git history

ArangoGutierrez
ArangoGutierrez reviewed Jun 15, 2022
View reviewed changes
go.mod
k8s.io/client-go v0.24.0
k8s.io/component-base v0.24.0
k8s.io/component-helpers v0.24.0
k8s.io/klog/v2 v2.60.1
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not 0.24, so this PR is updating more than a single dependency, the title of the PR says 0.24 (and doesn't mention of what)

Please enhance your PR description and Git commit to reflect all these changes

go.mod
k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
sigs.k8s.io/controller-runtime v0.11.1
sigs.k8s.io/controller-runtime v0.12.1-0.20220614131733-a759a0d51676
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

can't this be 0.12.1?

Copy link
Author

Choose a reason for hiding this comment

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

0.12.1 has not yet included the fixed pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, then let's not merge this until there's a release

/approve cancel

@ArangoGutierrez
Copy link
Contributor

I think Kubernetes should be backwards compatible and update references doesn't mean only this version and above are supported.

I am not sure. But there are always many compatibility issues in different places. Can you provide some compatibility tests in 1.23 with this PR(bump to 1.24)？

Kube is not good for this, even if you read the documentation for this project, Kueue only works 1.22 and up, 1.21 with some fnacy feature gate movements.
So we can not assume things will be backwards compatible.

@ArangoGutierrez
Copy link
Contributor

/hold

@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 Jun 15, 2022
@xiaoxubeii xiaoxubeii changed the title Bump to v0.24 Upgrade controller-runtime to fix deletion verb broken Jun 16, 2022
@xiaoxubeii xiaoxubeii changed the title Upgrade controller-runtime to fix deletion verb broken Upgrade controller-runtime to fix delete verb broken Jun 16, 2022
@xiaoxubeii
Copy link
Author

I think Kubernetes should be backwards compatible and update references doesn't mean only this version and above are supported.

I am not sure. But there are always many compatibility issues in different places. Can you provide some compatibility tests in 1.23 with this PR(bump to 1.24)？

Kube is not good for this, even if you read the documentation for this project, Kueue only works 1.22 and up, 1.21 with some fnacy feature gate movements. So we can not assume things will be backwards compatible.

OK, we can hold it until the right time to upgrade.

alculquicondor
alculquicondor reviewed Jun 16, 2022
View reviewed changes
Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

I'm not concerned about upgrading to 1.24 APIs. We never remove fields from k8s APIs, so it's generally backwards compatible.

/approve

go.mod Outdated
k8s.io/component-base v0.23.3
k8s.io/component-helpers v0.23.4
k8s.io/klog/v2 v2.40.1
k8s.io/api v0.24.0
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use 0.24.1?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I have updated to v0.24.1

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022
@alculquicondor
Copy link
Contributor

/retitle Upgrade controller-runtime and k8s packages

@k8s-ci-robot k8s-ci-robot changed the title Upgrade controller-runtime to fix delete verb broken Upgrade controller-runtime and k8s packages Jun 16, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xiaoxubeii
To complete the pull request process, please assign alculquicondor after the PR has been reviewed.
You can assign the PR to them by writing /assign @alculquicondor in a comment when ready.

The full list of commands accepted by this bot can be found 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2022
@alculquicondor
Copy link
Contributor

I'm going through the backlog of open PRs.

Feel free to reopen once there is a new version

/close

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Closed this PR.

In response to this:

I'm going through the backlog of open PRs.

Feel free to reopen once there is a new version

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@ArangoGutierrez ArangoGutierrez ArangoGutierrez requested changes

@denkensk denkensk Awaiting requested review from denkensk

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

None yet
5 participants
@xiaoxubeii @k8s-ci-robot @denkensk @ArangoGutierrez @alculquicondor