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.io dependencies to 1.17.2 #760

Merged
merged 1 commit into from
Jun 26, 2023
Merged

bump k8s.io dependencies to 1.17.2 #760

merged 1 commit into from
Jun 26, 2023

Conversation

MartinForReal
Copy link
Contributor

move k8s.io/kubernetes/pkg/ssh to k8s.io/node-problem-detector/test/e2e/lib/ssh/k8sssh because ssh package is removed from k8s.io/kubernetes
remove k8s.io/kubernetes from dependencies
bump k8s.io dependencies to 1.17.2

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 31, 2023
@MartinForReal
Copy link
Contributor Author

/assign @mmiranda96

mmiranda96
mmiranda96 reviewed May 31, 2023
View reviewed changes
Copy link
Contributor

@mmiranda96 mmiranda96 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I was working on something similar, but you beat me to it.

Could you upgrade to a later version of k8s.io? Perhaps 1.27, the latest available one.

go.mod
k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible
k8s.io/kubernetes v1.14.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing kubernetes as a dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the later version, we need to add a lot of replace statements. It is much simpler if there are fewer dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but not sure if this is correct. Let's keep it on the same version for now, we can send a follow-up PR just to update it.

Copy link
Contributor Author

@MartinForReal MartinForReal Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ssh package is the only package imported from k8s.io/kubernetes if we move the package, dependency can be removed.

@MartinForReal
Copy link
Contributor Author

Thank you for the review!
In k8s 1.19/1.18 release context. Context is introduced into the function parameters. I think we need to do the same to the NPD. And there will be less compatibility issues if the version gap is not that huge(api/behavior mismatch, package missing, function refactor and etc.). So, my plan would be upgrading to 1.19(adding context) and then upgrading to 1.24（to the oldest supported version ）How do you think?

@yaron-idan
Copy link

Hey @MartinForReal and @mmiranda96 - I was also looking into upgrading k8s dependencies to address #742, and opened this PR to perform the upgrade - #761. Do you think we can merge it instead to get closer to the current k8s version?

@MartinForReal
Copy link
Contributor Author

@mmiranda96 Which choice do you prefer? upgrade to 1.25 directly and put all of changes mentioned above into one pr?

@mmiranda96
Copy link
Contributor

Apologies for the delay. Since these upgrades result in very heavy PRs, I would prefer to review several smaller PRs (easier to review + Github page doesn't crash that often).

@MartinForReal
Copy link
Contributor Author

MartinForReal commented Jun 8, 2023 •
edited
Loading

Glad to know that.

Hey @MartinForReal and @mmiranda96 - I was also looking into upgrading k8s dependencies to address #742, and opened this PR to perform the upgrade - #761. Do you think we can merge it instead to get closer to the current k8s version?

I would suggest putting code changes into separate pr. @yaron-idan How do you think?

@yaron-idan
Copy link

@MartinForReal @mmiranda96 - so the suggestion is to merge this PR, and then merge the changes into #761 so it would include fewer changes? If so, I think it's a good idea.

@MartinForReal
Copy link
Contributor Author

MartinForReal commented Jun 9, 2023 •
edited
Loading

@mmiranda96 Could you please review this pr? Thanks!

mmiranda96
mmiranda96 suggested changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@mmiranda96 mmiranda96 left a comment

Choose a reason for hiding this comment

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

I left a couple of comments, please take a look. I'll continue reviewing the vendor/ directory, please bear with me as a PR this size will take me some time to review.

pkg/systemstatsmonitor/memory_collector_linux.go Outdated Show resolved Hide resolved
pkg/systemlogmonitor/logwatchers/filelog/translator.go Outdated Show resolved Hide resolved
pkg/systemstatsmonitor/net_collector_test.go Outdated Show resolved Hide resolved
test/e2e/lib/ssh/k8sssh/ssh.go Outdated Show resolved Hide resolved
go.mod
k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible
k8s.io/kubernetes v1.14.6
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but not sure if this is correct. Let's keep it on the same version for now, we can send a follow-up PR just to update it.

@MartinForReal
Copy link
Contributor Author

@mmiranda96 comment is addressed in new commit. Could you please kindly review this pr? Thanks!

@MartinForReal
Copy link
Contributor Author

/assign @andyxning

@MartinForReal
Copy link
Contributor Author

/assign @vteratipally

@MartinForReal
Copy link
Contributor Author

/assign @xueweiz

@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 26, 2023
@vteratipally
Copy link
Collaborator

/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 Jun 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinForReal, vteratipally

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

@MartinForReal
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit e6fbdd4 into kubernetes:master Jun 26, 2023
@MartinForReal MartinForReal mentioned this pull request Dec 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmiranda96 mmiranda96 mmiranda96 requested changes

@vteratipally vteratipally vteratipally approved these changes

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

@xueweiz xueweiz Awaiting requested review from xueweiz

Assignees

@andyxning andyxning

@xueweiz xueweiz

@mmiranda96 mmiranda96

@vteratipally vteratipally

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MartinForReal @yaron-idan @mmiranda96 @vteratipally @k8s-ci-robot @andyxning @xueweiz