replace golang.org/x/exp/inotify with standalone library #71011
Conversation
The inotify code was removed from golang.org/x/exp several years ago. Therefore importing it from that path prevents downstream consumers from using any module that makes use of more recent features of golang.org/x/exp. This change is a followup to google/cadvisor#2060 which was merged with kubernetes#70889 This fixes kubernetes#68478
|
Hi @sigma. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
/ok-to-test we need this in 1.13 since cadvisor has already moved to it /milestone v1.13 |
|
If we are creating a fork just for k8s.io/kubernetes (e.g. nothing in staging/ needs it), we usually drop code in https://github.com/kubernetes/kubernetes/tree/master/third_party/forked |
|
@mikedanese cadvisor needs it |
|
ok, thanks. |
|
/retest |
|
/priority important-soon |
|
@dims we should prioritize switching to https://github.com/fsnotify/fsnotify in the next release given that the existing library is unmaintained (unless we (k8s community) take on ownership). |
|
@vishh the last time we tried things broke - google/cadvisor#1807 someone has to go back and see if we can do something in fsnotify (or figure out if it has been fixed since) |
|
@dims I have bandwidth to review PRs and cut a new release |
|
out of curiosity, what's the expected benefit of having that code in 2 third_party locations? |
|
It is indeed ad-hoc (unless it is documented somewhere and I'm oblivious to
it). My understanding (and as @mikedanese mentioned earlier), we generally
fork dependencies that need to be forked (deleted, unmaintained) into
either third_party directory, or to a separate repo in k8s organization if
active development is expected (recent fork of glog is an example here).
@dims are you aware of any formal process?
…On Thu, Nov 15, 2018 at 12:52 PM Yann Hodique ***@***.***> wrote:
out of curiosity, what's the expected benefit of having that code in 2
third_party locations?
I'm still trying to figure out how we approach forking in general, it all
feels very ad-hoc so far :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71011 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGvIKLt8DBKBaKseXUPf8mxJUNBY4OyRks5uvdPwgaJpZM4Ycrte>
.
|
|
/approve I'm good with this PR as is, but if we change direction to something in third_party, I'd like to re-review. |
|
@thockin are we ok with taking on third party forks of deleted official golang repositories as dependencies? |
|
/kind bug /cc @thockin |
|
/remove-priority critical-urgent |
|
@AishSundar: Those labels are not set on the issue: In response to this:
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. |
|
@AishSundar personally I don't mind if it's pushed out. It should hopefully be a functionally neutral change, as the code in the old and new package is identical (it's slightly uglier not to merge it since said code is currently duplicated) |
|
@dims what do you think about this one? |
|
@nikopen i'd prefer that this gets in so k/k and cadvisor repos are in sync. (won't be terrible if it does not happen) |
|
Looks like this waiting on @thockin 's review @thockin can you please take a look |
|
I think we should just go ahead with this path as it's deduping to code that is already vendored. We can look in 1.14 if we want to move to fsnotify. At this point, this would just ensure that we don't have conflicting versions with cadvisor. /hold cancel |
|
/priority critical-urgent for merging needs an approval |
|
Manually approving for the kubelet. We're only changing the package import path to use the existing vendored code, but at a different path. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: cblecker, sigma 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The inotify code was removed from golang.org/x/exp several years ago. Therefore
importing it from that path prevents downstream consumers from using any module
that makes use of more recent features of golang.org/x/exp.
This change is a followup to google/cadvisor#2060 which was merged with #70889
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #68478
Special notes for your reviewer:
Does this PR introduce a user-facing change?: