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

Replace k8s.io/apimachinery/pkg/util/clock with k8s.io/utils/clock and bump k8s.io to 1.28 #793

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

MartinForReal
Copy link
Contributor

@MartinForReal MartinForReal commented Aug 17, 2023

  • Replace k8s.io/apimachinery/pkg/util/clock with k8s.io/utils/clock
  • bump k8s.io to 1.28

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 17, 2023
@MartinForReal MartinForReal force-pushed the master branch 3 times, most recently from 7fe24a2 to ef90641 Compare August 18, 2023 05:06
@MartinForReal
Copy link
Contributor Author

/retest

@MartinForReal MartinForReal force-pushed the master branch 2 times, most recently from d5b07af to 703904c Compare August 18, 2023 07:40
@MartinForReal MartinForReal changed the title Replace k8s.io/apimachinery/pkg/util/clock with k8s.io/utils/clock and bump k8s.io to 1.24 Replace k8s.io/apimachinery/pkg/util/clock with k8s.io/utils/clock and bump k8s.io to 1.28 Aug 18, 2023
@MartinForReal
Copy link
Contributor Author

/test pull-npd-e2e-node


@MartinForReal
Copy link
Contributor Author

/retest-required

@vteratipally
Copy link
Collaborator

/test pull-npd-e2e-node

1 similar comment
@vteratipally
Copy link
Collaborator

/test pull-npd-e2e-node

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2023
@MartinForReal MartinForReal force-pushed the master branch 2 times, most recently from b87c7e3 to 2210c50 Compare August 28, 2023 16:18
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2023
@MartinForReal
Copy link
Contributor Author

MartinForReal commented Aug 29, 2023

e2e error is related to kubernetes/kubernetes#120231

@MartinForReal
Copy link
Contributor Author

/test pull-npd-e2e-node


1 similar comment
@MartinForReal
Copy link
Contributor Author

/test pull-npd-e2e-node


@hakman
Copy link
Member

hakman commented Aug 29, 2023

/test pull-npd-e2e-node

2 similar comments
@hakman
Copy link
Member

hakman commented Aug 29, 2023

/test pull-npd-e2e-node

@hakman
Copy link
Member

hakman commented Aug 30, 2023

/test pull-npd-e2e-node

@MartinForReal
Copy link
Contributor Author

/test pull-npd-e2e-node


@MartinForReal
Copy link
Contributor Author

@hakman Thanks for the fix! I've also updated the pr. PTAL. Thanks!

Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

@MartinForReal I don't mind picking up the changes in this PR, but maybe keep the commit as cherry-pick (not very important though).

The commit message mentions k7s, which is a typo. In general, this is just a dependencies upgrade and maybe a separate commit to deal with the ticker change.

I don't think the changes you added for RealClock and FakeClock are needed:
https://github.com/kubernetes/utils/blob/3b25d923346b3814e0898684c97390092f31a61e/clock/clock.go#L82C1-L83C1

var _ = WithTicker(RealClock{})

// RealClock really calls time.Now()
type RealClock struct{}
...

@MartinForReal
Copy link
Contributor Author

@hakman Thanks for reviewing the pr!

@hakman
Copy link
Member

hakman commented Sep 1, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2023
k8s.io/component-base v0.17.2
k8s.io/api v0.28.1
k8s.io/apimachinery v0.28.1
k8s.io/client-go v9.0.0+incompatible
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also v0.28.1?

Suggested change
k8s.io/client-go v9.0.0+incompatible
k8s.io/client-go v0.28.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one is replaced with v0.28.1. please see the last line. It is related to cadvisor. I will try to upgrade that part in following pr.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. Thanks for the explanation.

Copy link
Member

Choose a reason for hiding this comment

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

Pretty strange to depend on cadvisor just for a log tailing lib 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has not changed for a long time..

@hakman
Copy link
Member

hakman commented Sep 5, 2023

/cc @vteratipally

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.

Sorry for the delay! Some small comments, but overall good. Thanks!
/lgtm

pkg/util/helpers_linux.go Show resolved Hide resolved
pkg/util/helpers_windows.go Show resolved Hide resolved
pkg/util/metrics/fakes.go Show resolved Hide resolved
pkg/util/metrics/fakes_test.go Show resolved Hide resolved
pkg/util/metrics/helpers.go Show resolved Hide resolved
pkg/util/metrics/metric.go Show resolved Hide resolved
pkg/util/metrics/metric_float64.go Show resolved Hide resolved
pkg/util/metrics/metric_int64.go Show resolved Hide resolved
@mmiranda96
Copy link
Contributor

Please ignore my previous comments, everything looks great!

@vteratipally
Copy link
Collaborator

sorry for the delay,

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinForReal, mmiranda96, 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

@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 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9ff6b0b into kubernetes:master Sep 7, 2023
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.

5 participants