🌱 Bump to k8s.io/* v0.36.0-alpha.2 & implement HasSyncedChecker()#3462
🌱 Bump to k8s.io/* v0.36.0-alpha.2 & implement HasSyncedChecker()#3462sbueringer wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
sbueringer
changed the title
sbueringer
force-pushed
the
pr-bump-v0.36-alpha.2
branch
2 times, most recently
from
8fe8bfa to
2a462f5
Compare
|
/hold I have to take a closer look and was running out of time today. I thought it's just a quick bump and a small change to the fakeInformer. |
k8s-ci-robot
added
the
do-not-merge/hold
sbueringer
force-pushed
the
pr-bump-v0.36-alpha.2
branch
4 times, most recently
from
1c5104b to
722804e
Compare
sbueringer
changed the title
|
/hold cancel /assign @alvaroaleman Should be ready now |
k8s-ci-robot
removed
the
do-not-merge/hold
…6.0-alpha.2 client-go v0.36.0-alpha.2 added HasSyncedChecker() to the ResourceEventHandlerRegistration interface. controller-runtime v0.23.1 does not implement this method, causing a build failure. Add a replace directive pointing to the upstream fix from kubernetes-sigs/controller-runtime#3462 (not yet merged/released). Also bumps k8s.io/apiextensions-apiserver to v0.36.0-alpha.2 via go mod tidy.
…a.2 (#84) * chore(deps): bump k8s.io/client-go from 0.36.0-alpha.1 to 0.36.0-alpha.2 Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.36.0-alpha.1 to 0.36.0-alpha.2. - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.36.0-alpha.1...v0.36.0-alpha.2) --- updated-dependencies: - dependency-name: k8s.io/client-go dependency-version: 0.36.0-alpha.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * fix(deps): patch controller-runtime compatibility with client-go v0.36.0-alpha.2 client-go v0.36.0-alpha.2 added HasSyncedChecker() to the ResourceEventHandlerRegistration interface. controller-runtime v0.23.1 does not implement this method, causing a build failure. Add a replace directive pointing to the upstream fix from kubernetes-sigs/controller-runtime#3462 (not yet merged/released). Also bumps k8s.io/apiextensions-apiserver to v0.36.0-alpha.2 via go mod tidy. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mr. Z <mrz1836@users.noreply.github.com>
| Synced bool | ||
| SyncedLock sync.Mutex | ||
| Synced bool | ||
| SyncedChecker cache.DoneChecker |
There was a problem hiding this comment.
Should we really make this entire thing configurable as opposed to implemented it based on Synced? This package is public so changing this later on would be a breaking change
There was a problem hiding this comment.
The question is how I can react on someone directly setting the Synced field to true (like we do today)
I see two options:
- unexporting synced and introducing a setter (setter then also closes the synced channel)
- Introducing a go routine that keeps checking Synced that closes the channel once Synced is true
I'm fine with both. First option sounds better to me, it's a breaking change but I'm fine with it
There was a problem hiding this comment.
Good point and yes, lets go with the setter 👍
There was a problem hiding this comment.
:/ This is even worse
We don't have a constructor, so no good place to make the channel.
Should I create a constructor and unexport the type?
Not sure if there is a good alternative to that.
I think it's not a good option to ask folks to initialize the struct with a channel.
I think it's also bad if I create the channel on demand in the setter of synced (what if someone calls the HasSyncedChecker method before that)
Maybe it could be okay to make the channel as part of Run? I'm not sure, feels brittle
There was a problem hiding this comment.
Should I create a constructor and unexport the type?
Not sure if there is a good alternative to that.
How would we make the setter available if the type is unexported?
Maybe it could be okay to make the channel as part of Run? I'm not sure, feels brittle
Makes sense to me insofar as that checking on the synced state before calling run would be wrong anyways, as an informer will never sync if it wasn't started
There was a problem hiding this comment.
How would we make the setter available if the type is unexported?
I guess exporting an interface, or is there another way to do this? (my goal is just to avoid incorrect instantiations of the type, apart from that I don't care about if the type is exported or not)
This would roughly align to the sharedIndexInformer.
There was a problem hiding this comment.
I guess exporting an interface, or is there another way to do this? (my goal is just to avoid incorrect instantiations of the type, apart from that I don't care about if the type is exported or not)
IMHO its okay to leave the type exported but say it must be constructructed through a constructor
There was a problem hiding this comment.
Alright, then let's do that
sbueringer
force-pushed
the
pr-bump-v0.36-alpha.2
branch
from
722804e to
984b9fc
Compare
Signed-off-by: Stefan Büringer buringerst@vmware.com
sbueringer
force-pushed
the
pr-bump-v0.36-alpha.2
branch
from
984b9fc to
4db5c73
Compare
|
@sbueringer: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Signed-off-by: Stefan Büringer buringerst@vmware.com