🌱 Downgrade github.com/evanphx/json-patch library to v4 to match upstream

[aerfio]

Changes an import of github.com/evanphx/json-patch to github.com/evanphx/json-patch/v5. Other codepaths use only json-patch/v5. It removes 1 direct dependency

[k8s-ci-robot]

Hi @aerfio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[vincepri]

/ok-to-test

[sbueringer]

/hold

This was intentional, see the godoc comment. Did upstream move to v5?

[aerfio]

I saw it, but I concluded that it was stale comment because in go.mod there was only v5/v5+incompatible. Now that I know there was some issue with newer versions of this lib (thanks @inteon) I'll research what to do next.

[sbueringer]

The reason we use the older version here was that we copied code from upstream and wanted it to use the same library as upstream.

I wasn't aware of general issues in the library. I wonder if we should re-visit our usage of v5 with this in mind.

(I didn't take a closer look yet)

[sbueringer]

cc @vincepri @alvaroaleman for awareness.

We should probably figure out if we have to do something before we cut v0.17

[aerfio]

kubernetes/kubernetes#120326 mentions that v5.6.0 does not include evanphx/json-patch@50fdc0b, but it's part of v5.7.0 btw

EDIT: I also saw kubernetes-sigs/kustomize#5479, same issue, they reverted back to v4

[sbueringer]

Maybe it's only if you use

github.com/evanphx/json-patch v5.6.0+incompatible

And not for

github.com/evanphx/json-patch/v5 v5.6.0

[sbueringer]

Okay so my interpretation:

• Kubernetes intentionally stayed on v4 because of: Update github.com/evanphx/json-patch to version v5.0.0 kubernetes/kubernetes#91622 (comment)
  • Basically because jsonpatch v5 became more RFC6902 compliant (Conform to RFC6902 replacement semantics evanphx/json-patch#85) and they didn't want to break Kubernetes users
• For the github.com/evanphx/json-patch module "v5.6.0+incompatible is older than v4.12.0+incompatible" (evanphx/json-patch used by 1.28 drops a fix included in 1.27 kubernetes/kubernetes#120326 (comment))
• client-go v0.29 is using v4.12.0+incompatible https://github.com/kubernetes/client-go/blob/v0.29.0/go.mod#L8
• we intentionally use github.com/evanphx/json-patch in fake/client.go because we copied code from https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194

Based on all this:

• I would "upgrade" github.com/evanphx/json-patch to v4.12.0+incompatible to be in sync with client-go v0.29
• I would keep our regular github.com/evanphx/json-patch/v5 dependency. I think it doesn't make sense for us to downgrade to become less compliant with RFC6902, now that we already made the move to /v5 in the past.
  • Note: We only use the jsonpatch/v5 dependency in this place:

    controller-runtime/pkg/client/patch.go

    Line 136 in a550f29

    return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)

[aerfio]

Ok, I've downgraded this dep to v4. I also copied the comment that you originally asked about to the go.mod, so that any changes to this line will be visible in the future. I hope this will prevent accidental changes

[sbueringer]

Thank you!

[inteon]

FYI: kubernetes/kubernetes#120327

[aerfio]

/retest

[aerfio]

Hmm the pull-controller-runtime-apidiff job fails with:

verifying api diff
invoking: 'hack/tools/bin/go-apidiff 984a121dbb8f6273eef55ebb4016e3b3deef1919 --print-compatible'
sigs.k8s.io/controller-runtime/pkg/reconcile
  Incompatible changes:
  - AsReconciler: changed from func(sigs.k8s.io/controller-runtime/pkg/client.Client, ObjectReconciler[T]) Reconciler to func(sigs.k8s.io/controller-runtime/pkg/client.Client, ObjectReconciler[T]) Reconciler
  - ObjectReconciler: changed from ObjectReconciler[T sigs.k8s.io/controller-runtime/pkg/client.Object] to ObjectReconciler[T sigs.k8s.io/controller-runtime/pkg/client.Object]

But there's no difference between changed to vs from. Is go-apidiff generics compatible? This PR is running a couple hours after #2592 was merged, which is the first (IIRC) public usage of generics in c-r, it might be related

[vincepri]

Try rebasing

[aerfio]

@vincepri already did that with the earlier force-push

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aerfio, vincepri

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:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

@sbueringer hold is yours to clear

[sbueringer]

Thx!

/lgtm

/hold cancel

[sbueringer]

/test ?

[k8s-ci-robot]

@sbueringer: The following commands are available to trigger required jobs:

• /test pull-controller-runtime-test

The following commands are available to trigger optional jobs:

• /test pull-controller-runtime-apidiff

Use /test all to run all jobs.

In response to this:

/test ?

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.

[sbueringer]

No idea why but tide was blocked because it though the hold label was there even though it wasn't. Force pushed the PR to force a re-run of everything

[sbueringer]

/lgtm

[k8s-ci-robot]

@aerfio: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	c01010e	link	false	/test pull-controller-runtime-apidiff

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.

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. I understand the commands that are listed here.

[aerfio]

Just an additional thought, if I understand this situation correctly: it might be nice to mention in the release notes that c-r users should downgrade github.com/evanphx/json-patch to v4.12.0 to match client-go (or upgrade all the way up to v5.7.0) so that the fix evanphx/json-patch@50fdc0b is included

[sbueringer]

@aerfio Good idea, will do