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.7.2 to 6.8.1 #34956

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

manusa
Copy link
Contributor

@manusa manusa commented Jul 24, 2023

This will require bumping the Fabric8 Kubernetes Client version in Dekorate first.

The Dekorate dependency is also bumped to latest 4.0.0 which includes the 6.8.0 compatible Kubernetes Client dependency.

/cc @metacosm

@manusa manusa changed the title Bump kubernetes-client-bom from 6.7.2 to 6.8.0 Bump kubernetes-client-bom from 6.7.2 to 6.8.1 Aug 14, 2023
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Sep 7, 2023
@manusa manusa marked this pull request as ready for review September 7, 2023 04:56
@quarkus-bot

This comment has been minimized.

@manusa manusa marked this pull request as draft September 7, 2023 05:20
@manusa manusa marked this pull request as ready for review September 7, 2023 07:35
@metacosm
Copy link
Contributor

metacosm commented Sep 7, 2023

Tried this PR locally, this is definitely not a backwards compatible change: some of my code doesn't compile anymore.

@manusa
Copy link
Contributor Author

manusa commented Sep 7, 2023

Could you provide more details about your specific compilation failures?

Is it related to the change of syntax in builders? Or in intermediate type signatures (fluents)?

@quarkus-bot

This comment has been minimized.

@metacosm
Copy link
Contributor

metacosm commented Sep 7, 2023

Yes, issues with intermediate signatures with fluents, things like NamedInstallStrategyFluent.SpecNested<ClusterServiceVersionSpecFluent.InstallNested<ClusterServiceVersionFluent.SpecNested<ClusterServiceVersionBuilder>>> used as a method parameter…

@manusa
Copy link
Contributor Author

manusa commented Sep 7, 2023

You'll notice how these can be fixed if you check the changes in this PR.

I'm not sure how we should document this in Quarkus to help user's migration

@metacosm
Copy link
Contributor

metacosm commented Sep 7, 2023

Not that easy in my case… Then again, I can't already not read or understand the explicit type as it is. I applied the suggestion from my IDE and the type got changed to:

NamedInstallStrategyFluent<ClusterServiceVersionSpecFluent<ClusterServiceVersionFluent<ClusterServiceVersionBuilder>.SpecNested<ClusterServiceVersionBuilder>>.InstallNested<ClusterServiceVersionFluent<ClusterServiceVersionBuilder>.SpecNested<ClusterServiceVersionBuilder>>>.SpecNested<ClusterServiceVersionSpecFluent<ClusterServiceVersionFluent<ClusterServiceVersionBuilder>.SpecNested<ClusterServiceVersionBuilder>>.InstallNested<ClusterServiceVersionFluent<ClusterServiceVersionBuilder>.SpecNested<ClusterServiceVersionBuilder>>>

I mean, come on… that's completely not understandable (not that the original type was) but now it's significantly worse. I mean I wouldn't be able to make that change without assistance from the IDE. Not sure what we can do here but we're definitely not going in the right direction on this particular issue.

@metacosm
Copy link
Contributor

metacosm commented Sep 8, 2023

I managed to get it dealt with by using NamedInstallStrategyFluent<?>.SpecNested<?> instead, which I guess makes sense.

@metacosm
Copy link
Contributor

metacosm commented Sep 14, 2023

Where do we stand on this? Has dekorate 4.0.0 been released as I don't see it in the list of releases? /cc @manusa @iocanel

@manusa
Copy link
Contributor Author

manusa commented Sep 14, 2023

From my standing point the pull request is ready (it included the 4.0.0 Dekorate bump one week ago) , not sure if it's expected for something else to be added.

@metacosm
Copy link
Contributor

Can we get this merged then? @iocanel @geoand

@geoand
Copy link
Contributor

geoand commented Sep 14, 2023

I have no objections, but I would like the PR rebased onto main since the last time we had a CI run was over a week ago.

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

geoand commented Sep 14, 2023

I assume this is a somewhat breaking change, right?

@metacosm
Copy link
Contributor

I assume this is a somewhat breaking change, right?

Yes, so probably for 3.5.

@metacosm
Copy link
Contributor

@manusa minor nit picking but the commit message for the second commit is wrong: it should say dekorate, not kubernetes client bom.

@manusa
Copy link
Contributor Author

manusa commented Sep 14, 2023

I assume this is a somewhat breaking change, right?

There are breaking changes with Sundrio generated classes (builders).

They can be mostly tackled by using type inference or by following the approach I did in this PR (#34956 (comment)) (for intermediate variables).

Also some builder methods are no longer available. However, they were already deprecated.

I probably asked this internally (as I can't find the comment here). Maybe we want to add some entries for the migration process.

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

quarkus-bot bot commented Sep 14, 2023

Failing Jobs - Building cac2df1

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 20

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/smallrye-graphql/deployment 
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/opentelemetry/deployment and 38 more

📦 extensions/smallrye-graphql/deployment

io.quarkus.smallrye.graphql.deployment.RequestLeakDetectionTest.testWithConcurrentCalls line 55 - More details - Source on GitHub

BUG! exception in phase 'class generation' in source unit 'Script1.groovy' unexpected NullPointerException
	at org.codehaus.groovy.control.CompilationUnit$IPrimaryClassNodeOperation.doPhaseOperation(CompilationUnit.java:942)
	at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:692)

@geoand geoand merged commit 8625c06 into quarkusio:main Sep 15, 2023
50 of 51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 15, 2023
@michalvavrik
Copy link
Member

michalvavrik commented Sep 18, 2023

After this PR, when I use io.fabric8.kubernetes.client.impl.KubernetesClientImpl#load to load my template

---
apiVersion: "v1"
kind: "List"
items:
- apiVersion: "v1"
  kind: "Service"
  metadata:
    name: "app"
  spec:
    ports:
    - name: "http"
      port: 8080
      targetPort: 8080
    selector:
      app.kubernetes.io/name: "app"
    type: "ClusterIP"
- apiVersion: "v1"
  kind: "Service"
  metadata:
    name: "app-management"
  spec:
    ports:
      - name: "management"
        port: 9000
        targetPort: 8080
    selector:
      app.kubernetes.io/name: "app"
    type: "ClusterIP"
- apiVersion: "apps.openshift.io/v1"
  kind: "DeploymentConfig"
  metadata:
    labels:
      app.openshift.io/runtime: "quarkus"
      app.kubernetes.io/name: "app"
    name: "app"
  spec:
    replicas: 1
    selector:
      app.kubernetes.io/name: "app"
    template:
      metadata:
        labels:
          app.openshift.io/runtime: "quarkus"
          app.kubernetes.io/name: "app"
      spec:
        containers:
        - env:
          - name: "KUBERNETES_NAMESPACE"
            valueFrom:
              fieldRef:
                fieldPath: "metadata.namespace"
          image: "my-registry"
          name: "app"
          ports:
            - containerPort: 8080
              name: "http"
              protocol: "TCP"
              ports:
            - containerPort: 8080
              name: "management"
              protocol: "TCP"

my deployment config spec contains empty triggers and then when I adjust stuff and serialize it as KubernetesList, I can see newly template contains triggers: []

- apiVersion: "apps.openshift.io/v1"
  kind: "DeploymentConfig"
  metadata:
    labels:
      app.openshift.io/runtime: "quarkus"
      app.kubernetes.io/name: "app"
      scenarioId: "OpenShiftUsingContainerRegistryPingPongResourceIT-1695036746"
    name: "app"
    namespace: "mvavrik-debug"
  spec:
    replicas: 1
    selector:
      app.kubernetes.io/name: "app"
    template:
      metadata:
        labels:
          app.openshift.io/runtime: "quarkus"
          app.kubernetes.io/name: "app"
          tsLogWatch: "app"
          scenarioId: "OpenShiftUsingContainerRegistryPingPongResourceIT-1695036746"
        namespace: "mvavrik-debug"
      spec:
        containers:
        - env:
          - name: "KUBERNETES_NAMESPACE"
            valueFrom:
              fieldRef:
                fieldPath: "metadata.namespace"
          - name: "quarkus.log.console.format"
            value: "%d{HH:mm:ss,SSS} %s%e%n"
          image: "my-registry"
          name: "app"
          ports:
          - containerPort: 8080
            name: "http"
            protocol: "TCP"
            ports: null
          - containerPort: 8080
            name: "management"
            protocol: "TCP"
    triggers: []

which prevents my rollout with oc: error: #1 is already in progress (Pending).. This is new behavior, why are triggers: [] added, please?

@michalvavrik
Copy link
Member

nope, I got rid of triggers and still having issue (with the bump only), so it's not related to the triggers.

@manusa
Copy link
Contributor Author

manusa commented Sep 18, 2023

cc/ @shawkins

There were some changes in the serialization default behavior, let me try to find some references.

@manusa
Copy link
Contributor Author

manusa commented Sep 18, 2023

Fix #5262: Built-in resources were in-consistent with respect to their serialization or empty collections. In many circumstances this was confusing behavior. In order to be consistent all built-in resources will omit empty collections by default. This is a breaking change if you are relying on an empty collection in a json merge or a strategic merge where the list has a patchStrategy of atomic. In these circumstances the empty collection will no longer be serialized. You may instead use a json patch, server side apply instead, or modify the serialized form of the patch.

fabric8io/kubernetes-client#5262

AFAIR setting the triggers to null (when "adjusting stuff") should fix your problem.

https://github.com/manusa/yakd/blob/f750188140c3706ed92ce51f111a7087484b7b56/src/main/java/com/marcnuri/yakd/deploymentconfigs/DeploymentConfigService.java#L89-L93

@manusa manusa deleted the deps/kubernetes-client branch September 18, 2023 13:16
@michalvavrik
Copy link
Member

Fix #5262: Built-in resources were in-consistent with respect to their serialization or empty collections. In many circumstances this was confusing behavior. In order to be consistent all built-in resources will omit empty collections by default. This is a breaking change if you are relying on an empty collection in a json merge or a strategic merge where the list has a patchStrategy of atomic. In these circumstances the empty collection will no longer be serialized. You may instead use a json patch, server side apply instead, or modify the serialized form of the patch.

fabric8io/kubernetes-client#5262

AFAIR setting the triggers to null (when "adjusting stuff") should fix your problem

yeah, it did. and after that, diff on previously serialized and newly (after bump) serialized deployment config doesn't show changes. I have some other issue caused by this bump though, I don't know where so probably not right time to get you involved :-)

Thanks for prompt answer.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants