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

Prepare clock package to be compatible replacement of apimachinery/util/clock #224

Merged

Conversation

MadhavJivrajani
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani commented Oct 11, 2021

What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Commit 1:
The reason for doing this is that utils/clock is supposed
to be a full replacement for k8s.io/apimachinery/pkg/util/clock.

Currently, WithDelayedExecution extends Clock - in order to deprecate
apimachinery/clock, WithDelayedExecution would be missing the NewTicker
method if gradual deprecation via type aliasing was to be done.

By extending WithTicker instead of Clock, we solve this issue.

Commit 2:
Eventhough the implementation is just a panic,
re-implementing the NewTicker method helps
in migration of the apimachinery/util/clock pkg
to the utils/clock pkg in terms of having identical
clients for IntervalClock in both packages as
well as the IntervalClock implementing the
WithTickerAndDelayedExcution interface.

Which issue(s) this PR fixes:
Related to kubernetes/kubernetes#94738

Special notes for your reviewer:
The next step after this would be to implement the Tick method in apimachinery clock, type aliasing the interfaces and marking the package as deprecated.

Release note:

NONE

/assign @liggitt @dims
/cc @wojtek-t

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 11, 2021
@k8s-ci-robot k8s-ci-robot requested a review from wojtek-t October 11, 2021 12:31
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 11, 2021
@liggitt
Copy link
Member

liggitt commented Oct 11, 2021

this has the same problem with making incompatible changes to existing interfaces as #221 (comment)

@MadhavJivrajani
Copy link
Contributor Author

MadhavJivrajani commented Oct 11, 2021

The API diff will fail here. Do we introduce a new interface to further encapsulate things?
Considering that this change was done only a few days back, is it possible to somehow get this in or would there be a better way to go about it?

@wojtek-t
Copy link
Member

this has the same problem with making incompatible changes to existing interfaces as #221 (comment)

@liggitt - This is something we may want to consider, as this type was introduced couple days ago.
I don't have strong opinion though - just pointing out.

@liggitt
Copy link
Member

liggitt commented Oct 11, 2021

@liggitt - This is something we may want to consider, as this type was introduced couple days ago.

merged in #221 ~2 weeks ago, right?

@MadhavJivrajani
Copy link
Contributor Author

Yep :)

@MadhavJivrajani
Copy link
Contributor Author

Hey @liggitt - any updates on this?

@liggitt
Copy link
Member

liggitt commented Oct 13, 2021

once an interface is merged, available, and picked up as a dependency in other places, we should not break the interface

https://grep.app/search?q=WithDelayedExecution shows the existing interface has already been picked up by some repos other than kubernetes/kubernetes

@MadhavJivrajani
Copy link
Contributor Author

Ah I see - thanks for pointing towards that, I'll revert the change and introduce another interface.
Sorry about the back and forth on this

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
@MadhavJivrajani MadhavJivrajani force-pushed the rectify-interface-extension branch from 280f528 to ba42345 Compare October 22, 2021 06:26
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 22, 2021
@MadhavJivrajani
Copy link
Contributor Author

xposting: kubernetes/kubernetes#94738 (comment)

Eventhough the implementation is just a panic,
re-implementing the `NewTicker` method helps
in migration of the apimachinery/util/clock pkg
to the utils/clock pkg in terms of having identical
clients for `IntervalClock` in both packages as
well as the `IntervalClock` implementing the
`WithTickerAndDelayedExcution` interface.

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
@MadhavJivrajani MadhavJivrajani changed the title Replace Clock with WithTicker in WithDelayedExecution Prepare clock package to be compatible replacement of apimachinery/util/clock Dec 3, 2021
@wojtek-t
Copy link
Member

wojtek-t commented Dec 3, 2021

/lgtm

@liggitt @dims - can you take a look?

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

/hold
(would like to double check once again post approval)

@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 Dec 3, 2021
@dims
Copy link
Member

dims commented Dec 3, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, MadhavJivrajani, wojtek-t

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 Dec 3, 2021
@liggitt
Copy link
Member

liggitt commented Dec 6, 2021

can you open a WIP PR against kubernetes/kubernetes that bumps k8s.io/utils to this commit and ensures we can completely switch over to this package and get green CI on that PR?

you can do that in kubernetes/kubernetes like this:

hack/pin-dependency.sh k8s.io/utils=github.com/MadhavJivrajani/utils rectify-interface-extension
hack/update-vendor.sh

@MadhavJivrajani
Copy link
Contributor Author

@wojtek-t
Copy link
Member

wojtek-t commented Dec 7, 2021

That LGTM

@MadhavJivrajani
Copy link
Contributor Author

Thank you, both!
Removing hold now seeing that the CI in the test PR is green :)
/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 Dec 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7d6a63d into kubernetes:master Dec 8, 2021
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants