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

bump k8s to 1.22 #169

Merged
merged 2 commits into from
Sep 16, 2021
Merged

bump k8s to 1.22 #169

merged 2 commits into from
Sep 16, 2021

Conversation

atoato88
Copy link
Contributor

@atoato88 atoato88 commented Aug 17, 2021 •
edited
Loading

What this PR does / why we need it:

This PR bumps k8s modules to 1.22.0

Special notes for your reviewer:

If this PR is too fast to merge because of other repository doesn't bump modules to 1.22, please add /hold tag.

Update:
I close this PR and will create other PR once controller-runtime release-0.10 tag is released.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2021
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2021
@atoato88
Copy link
Contributor Author

Oh, should I update controller-runtime version in this PR?

@justinsb
Copy link
Contributor

justinsb commented Aug 17, 2021 •
edited
Loading

Thanks for doing this @atoato88 ... I do think you're right in that we should try to follow the same k8s version as the controller-runtime version we're using. My reasoning is that because client-go doesn't use true go module versioning, if there was a breaking change between client-go 1.21 and 1.22, and we were using 1.22 but controller-runtime was using 1.21, we might end up where the code can't be compiled.

It does look like controller-runtime was recently updated (in kubernetes-sigs/controller-runtime#1626), but it also looks like that version hasn't yet been tagged.

I suggest once the version of controller-runtime is tagged, that we merge this change and update controller-runtime at the same time. However, we could update controller-runtime now by pinning to a SHA...

I also suggest that we should start thinking about versioning and tagging in this repository also. I think it is probably easiest to align with controller runtime. which describes their strategy here: https://github.com/kubernetes-sigs/controller-runtime/blob/master/VERSIONING.md But in short I suggest we maintain a release-0.9 branch that works with controller-runtime-0.9, and in future a release-0.10 branch that works with controller-runtime 0.10.

If we want to do that, the big upside of doing that would be that master would probably be the 0.10 branch before it is released, which would use a SHA reference to the controller-runtime master branch.

That's a lot of words to say I think (1) we should think about release branches here and (2) if we are going to do release branches, we can merge this PR (but you should also update controller-runtime to the latest from the master branch in this PR).

@atoato88
Copy link
Contributor Author

Thanks for doing this @atoato88 ... I do think you're right in that we should try to follow the same k8s version as the controller-runtime version we're using. My reasoning is that because client-go doesn't use true go module versioning, if there was a breaking change between client-go 1.21 and 1.22, and we were using 1.22 but controller-runtime was using 1.21, we might end up where the code can't be compiled.
It does look like controller-runtime was recently updated (in kubernetes-sigs/controller-runtime#1626), but it also looks like that version hasn't yet been tagged.

Yes I agree with you and my PR was too fast timing. 😅

I suggest once the version of controller-runtime is tagged, that we merge this change and update controller-runtime at the same time. However, we could update controller-runtime now by pinning to a SHA...

Yes, I think that we should update modules for this repository once controller-runtime is tagged.

As of now, should I close this PR and create other PR on that time? or hold this PR temporary?

@justinsb
Copy link
Contributor

As of now, should I close this PR and create other PR on that time? or hold this PR temporary?

Actually I think if you want to update this PR to pull in the latest controller-runtime on the main/master branch (which uses k8s 1.22), then I think we can merge this. If we agree that the versioning procedure I outline above makes sense, our master branch will become release-0.10 once controller-runtime tags their release-0.10.

We can also take a release-0.9 branch from immediately before this PR merges, if we decide we need that. But I think just aiming for a solid release-0.10 (and then tagging 0.10.0 etc from it) is probably better.

@atoato88
Copy link
Contributor Author

Actually I think if you want to update this PR to pull in the latest controller-runtime on the main/master branch (which uses k8s 1.22), then I think we can merge this. If we agree that the versioning procedure I outline above makes sense, our master branch will become release-0.10 once controller-runtime tags their release-0.10.

The motivation that I create this PR is catch up with latest controller-runtime which is tagged.
Previously, some PR bumping some modules was along with same policy, I think.

I agree with your versioning procedure comment.
So, I close this PR and create other PR once controller-runtime release-0.10 tag is released.

@atoato88
Copy link
Contributor Author

/close
Thank you to discussion.
I close this PR and will create other PR once controller-runtime release-0.10 tag is released.

@k8s-ci-robot
Copy link
Contributor

@atoato88: Closed this PR.

In response to this:

/close
Thank you to discussion.
I close this PR and will create other PR once controller-runtime release-0.10 tag is released.

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.

@justinsb justinsb mentioned this pull request Aug 31, 2021
Closed
@varshaprasad96
Copy link
Member

@atoato88 controller-runtime released 0.10.0 just yesterday. Can we reopen the PR?

varshaprasad96
varshaprasad96 reviewed Sep 1, 2021
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

As the other projects have moved to go 1.16, can we also update the go version in declarative pattern? Looks like we are still on 1.15

@atoato88
Copy link
Contributor Author

atoato88 commented Sep 3, 2021

/reopen
Latest controller-runtime which taged v0.10.0 is released.
I reopen this PR.

@k8s-ci-robot k8s-ci-robot reopened this Sep 3, 2021
@k8s-ci-robot
Copy link
Contributor

@atoato88: Reopened this PR.

In response to this:

/reopen
Latest controller-runtime which taged v0.10.0 is released.
I reopen this 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.

@atoato88 atoato88 mentioned this pull request Sep 3, 2021
Merged
@atoato88
Copy link
Contributor Author

atoato88 commented Sep 7, 2021

/retest

#175 is merged, I retest on this PR.

@atoato88
Copy link
Contributor Author

atoato88 commented Sep 8, 2021

Oh, it isn't prow job..
I've rebased now. 😅

estroz
estroz approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@estroz estroz 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

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atoato88, estroz

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 Sep 16, 2021
@estroz
Copy link
Contributor

estroz commented Sep 16, 2021

/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 Sep 16, 2021
@estroz
Copy link
Contributor

estroz commented Sep 16, 2021

/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 Sep 16, 2021
@k8s-ci-robot k8s-ci-robot merged commit 497ee89 into kubernetes-sigs:master Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varshaprasad96 varshaprasad96 varshaprasad96 left review comments

@estroz estroz estroz approved these changes

@justinsb justinsb Awaiting requested review from justinsb

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@estroz estroz

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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atoato88 @justinsb @k8s-ci-robot @varshaprasad96 @estroz