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

Bump kubernetes-client-bom from 6.2.0 to 6.3.1 #29807

Merged
merged 3 commits into from Dec 21, 2022

Conversation

manusa
Copy link
Contributor

@manusa manusa commented Dec 12, 2022

Kubernetes Client 6.3.0 was just released: https://github.com/fabric8io/kubernetes-client/releases/tag/v6.3.0

Just speeding up the dependabot process to see if CI reports any issue.

/cc @metacosm

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/kubernetes labels Dec 12, 2022
@quarkus-bot

This comment has been minimized.

@manusa
Copy link
Contributor Author

manusa commented Dec 12, 2022

Failures seem related to K8s, will look into these tomorrow.

@manusa
Copy link
Contributor Author

manusa commented Dec 13, 2022

Putting this on hold since there might be a regression affecting the client: fabric8io/kubernetes-client#4666

@manusa manusa marked this pull request as draft December 13, 2022 08:19
@geoand
Copy link
Contributor

geoand commented Dec 13, 2022

@manusa do you think it would make sense to run some of these Quarkus tests on the Fabric8 k8s Client side before releasing with the intent of catching issues like this?

@manusa
Copy link
Contributor Author

manusa commented Dec 13, 2022

@manusa do you think it would make sense to run some of these Quarkus tests on the Fabric8 k8s Client side before releasing with the intent of catching issues like this?

The regression has nothing to do with these failures. I haven't checked, but probably these are related to a change we did in the serialization generic behavior. If this is the case, then I'm still thinking what the best approach is.

I will check which of these tests might make sense to move upstream too, but it's very likely that we have equivalent tests upstream. The problem is that we are doing subtle changes to the generic behavior in each release. Some of these changes can be tackled downstream so they won't affect users, and others need to be communicated to users. The tests in Quarkus allow us to detect and decide on that.

@geoand
Copy link
Contributor

geoand commented Dec 13, 2022

👌🏼

Signed-off-by: Marc Nuri <marc@marcnuri.com>
@manusa manusa changed the title Bump kubernetes-client-bom from 6.2.0 to 6.3.0 Bump kubernetes-client-bom from 6.2.0 to 6.3.1 Dec 20, 2022
Since Fabric8 Kubernetes Client 6.3, types are no longer
automatically registered for deserialization.
For automatic type registration an
`META-INF/services/io.fabric8.kubernetes.api.model.KubernetesResource`
SPI definition file needs to be provided instead with the
Kubernetes Resources (Objects -HasMadata impls-) that need to be
automatically registered in the KubernetesDeserializer.

fabric8io/kubernetes-client#3923
Signed-off-by: Marc Nuri <marc@marcnuri.com>
@manusa manusa marked this pull request as ready for review December 20, 2022 11:45
@manusa manusa requested a review from geoand December 20, 2022 11:45
@manusa
Copy link
Contributor Author

manusa commented Dec 20, 2022

Since Fabric8 Kubernetes Client 6.3, types are no longer automatically registered for deserialization.
For automatic type registration an META-INF/services/io.fabric8.kubernetes.api.model.KubernetesResource SPI definition file needs to be provided instead with the Kubernetes Resources (Objects -HasMadata impls-) that need to be
automatically registered in the KubernetesDeserializer.

To fix the test problems, I basically adapted the tests to expect a GenericKubernetesResource instead of a ServiceBinding. I'm not sure if this is OK.

The basic implications from the current Kubernetes Client version is that YAMLs containing a ServiceBinding definition will no longer be deserialized into a ServiceBinding. For most usage cases this should be fine, but you should confirm if this is OK.

@iocanel @Sgitario

fabric8io/kubernetes-client#3923

@geoand
Copy link
Contributor

geoand commented Dec 20, 2022

@iocanel do we interact with any API server that uses ServiceBinding?

@iocanel
Copy link
Contributor

iocanel commented Dec 20, 2022

No, but we do generate ServiceBinding resources and users may provide their own too. In the latter case we would need to Deserialize. Given the adoption of A SB this is no biggie.

@geoand
Copy link
Contributor

geoand commented Dec 20, 2022

users may provide their own too

Ah, good point. But we should at least have an entry in the migration guide for people that do intend to do this

@manusa
Copy link
Contributor Author

manusa commented Dec 20, 2022

No, but we do generate ServiceBinding resources and users may provide their own too. In the latter case we would need to Deserialize. Given the adoption of A SB this is no biggie.

For this case, Dekorate should provide the SPI definition file for the ServiceBinding class.
In the client we do this using Sundr.io's TemlateTransformation annotation:
https://github.com/fabric8io/kubernetes-client/blob/789cce92eadc28419393f4180637bd19a15355bf/extensions/camel-k/model-v1/src/generated/java/io/fabric8/camelk/v1/CamelCatalog.java#L69-L71

This generates a file target/classes/META-INF/services/io.fabric8.kubernetes.api.model.KubernetesResource with the following content:

io.fabric8.camelk.v1.IntegrationKitList
io.fabric8.camelk.v1.IntegrationPlatformList
io.fabric8.camelk.v1.Integration
io.fabric8.camelk.v1.IntegrationKit
io.fabric8.camelk.v1.IntegrationPlatform
io.fabric8.camelk.v1.CamelCatalogList
io.fabric8.camelk.v1.IntegrationList
io.fabric8.camelk.v1.Build
io.fabric8.camelk.v1.BuildList
io.fabric8.camelk.v1.CamelCatalog

This just reminded me that I need to add these 🤦 to the extension too.

Signed-off-by: Marc Nuri <marc@marcnuri.com>
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 20, 2022

Failing Jobs - Building 4c3c083

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@iocanel
Copy link
Contributor

iocanel commented Dec 21, 2022

For this case, Dekorate should provide the SPI definition file for the ServiceBinding class.
Hmmm, didn't realize that ServiceBinding was not defined in a k8s client extension but in dekorate. Yeah, that makes sense.

Raised an issue on dekorate: dekorateio/dekorate#1116

@gsmet
Copy link
Member

gsmet commented Dec 21, 2022

Is this one good to go or we need to wait for the Dekorate upgrade?

@iocanel
Copy link
Contributor

iocanel commented Dec 21, 2022

Is this one good to go or we need to wait for the Dekorate upgrade?

The dekorate fix is already merged in dekorate, so we could have a release later today.

But I wouldn't mind letting it in even without the fix, I really doubt that there are users out there that bring their own
ServiceBinding resources.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

lgtm

@gsmet gsmet merged commit 3e68178 into quarkusio:main Dec 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 21, 2022
@manusa manusa deleted the deps/kubernetes-client branch December 21, 2022 11:40
Sgitario added a commit to Sgitario/quarkus that referenced this pull request Dec 21, 2022
ebullient pushed a commit to maxandersen/quarkus that referenced this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/kubernetes release/breaking-change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants