search

joan38 / kubernetes-client

A Kubernetes client for Scala
Apache License 2.0
122 stars 41 forks source link

Watch from resource version #251

Closed jchapuis closed 12 months ago

jchapuis commented 12 months ago

Fixes https://github.com/joan38/kubernetes-client/issues/138

@timbertson I have taken the liberty of simplifying the diff and correcting the tests in WatchableTests, were they ever passing?

I need this exact pattern of retrieving the list and then watching from a revision @joan38, so I've done this so that this can maybe make the cut before the upcoming release? 🤞

Note that in the newer v1.27 beta API there is support for streaming lists: https://kubernetes.io/docs/reference/using-api/api-concepts/#streaming-lists which would be even better

yurique commented 12 months ago

I think this is the same as here: https://github.com/joan38/kubernetes-client/pull/223/files

jchapuis commented 12 months ago

I think this is the same as here: https://github.com/joan38/kubernetes-client/pull/223/files

@yurique yes i've basically resumed from there and made the tests pass (+ simplified a bit the diff)

timbertson commented 12 months ago

Hmm I thought the tests were all passing when I raised it. It's been a while, I'll take another look soon

timbertson commented 12 months ago

Looks like @joan38 has pushed to my original PR since I raised it, so I'm planning to get that working and then clean up the diff if necessary, it doesn't seem that useful to have two nearly-identical diffs with the tests failing.

joan38 commented 12 months ago

Thanks guys, I was trying to do some modifications on the other PR and buried myself in failed tests. After we get this merged I will make a release.

joan38 commented 12 months ago

Tests failing with 422 Unprocessable Content

yurique commented 12 months ago

which of the PRs should we be focusing on? :) this one or the initiail https://github.com/joan38/kubernetes-client/pull/223?

interestingly, tests are currently failing in both, but with different status codes (422 here vs 409 there)

joan38 commented 12 months ago

🤷🏻‍♂️ But I'll make sure to credit all of you in the release either way.

yurique commented 12 months ago

I've posted a fix to the failing test to the other PR, in a comment (didn't want to open a third PR :) )

yurique commented 12 months ago

in the newer v1.27 beta API there is support for streaming lists

Do we only need to add the sendInitialEvents url param to enable this new feature? If so, it should be ease to add, right?

jchapuis commented 12 months ago

Closing this, sorry was trying to help but pushed too fast and didn't see that CRD instanciation of the tests somehow behaved different and had a failure 🙈

jchapuis commented 12 months ago

in the newer v1.27 beta API there is support for streaming lists

Do we only need to add the sendInitialEvents url param to enable this new feature? If so, it should be ease to add, right?

yes looks like it, although the docs say

When you set sendInitialEvents=true in the query string, Kubernetes also requires that you set resourceVersionMatch to NotOlderThan value

not yet familiar with the lib, this would only work with 1.27 so how would we go about exposing it? adding a def listAndWatch to Watchable and just return whatever failure is returned by kube when not supported?

yurique commented 12 months ago

From version v1.19, Kubernetes API servers also support the resourceVersionMatch parameter on list requests.

I guess I would just add optional sendInitialEvents and resourceVersionMatch parameters, just like resourceVersion, and let the user of the library and their k8s figure it out 🤔