🐛 update jsonpatch

[kdada]

To fix gomodules/jsonpatch#16.

This bug may cause wrong patches when modifying arrays in webhook.

Example:

Origin:

{
  "apiVersion": "kubedb.com/v1alpha1",
  "kind": "Elasticsearch",
  "metadata": {
    "name": "quick-elasticsearch",
    "namespace": "demo"
  },
  "spec": {
    "tolerations": [
      {
          "key": "node.kubernetes.io/key1",
          "operator": "Equal",
          "value": "value1",
          "effect": "NoSchedule"
      },
      {
          "key": "node.kubernetes.io/key2",
          "operator": "Equal",
          "value": "value2",
          "effect": "NoSchedule"
      },
      {
          "key": "node.kubernetes.io/not-ready",
          "operator": "Exists",
          "effect": "NoExecute",
          "tolerationSeconds": 300
      },
      {
          "key": "node.kubernetes.io/unreachable",
          "operator": "Exists",
          "effect": "NoExecute",
          "tolerationSeconds": 300
      }
    ]
  }
}

Target:

{
  "apiVersion": "kubedb.com/v1alpha1",
  "kind": "Elasticsearch",
  "metadata": {
    "name": "quick-elasticsearch",
    "namespace": "demo"
  },
  "spec": {
    "tolerations": [
      {
          "key": "node.kubernetes.io/key2",
          "operator": "Equal",
          "value": "value2",
          "effect": "NoSchedule"
      },
      {
          "key": "node.kubernetes.io/key1",
          "operator": "Equal",
          "value": "value1",
          "effect": "NoSchedule"
      }
    ]
  }
}

The jsonpatch will generate patches:

[
    {
        "op": "remove",
        "path": "/spec/tolerations/3"
    },
    {
        "op": "replace",
        "path": "/spec/tolerations/2/key",
        "value": "node.kubernetes.io/key1"
    },
    {
        "op": "replace",
        "path": "/spec/tolerations/2/value",
        "value": "value1"
    },
    {
        "op": "remove",
        "path": "/spec/tolerations/0"
    }
]

[k8s-ci-robot]

Welcome @kdada!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[tamalsaha]

It should be gomodules.xyz/jsonpatch/v2 v2.0.0

[tamalsaha]

gomodules.xyz/jsonpatch/v2 everywhere.

[kdada]

@tamalsaha PTAL

[tamalsaha]

/lgtm

[k8s-ci-robot]

@tamalsaha: changing LGTM is restricted to assignees, and only kubernetes-sigs/controller-runtime repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

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.

[kdada]

We should declare that this error could cause the user cluster to chaos. Users should update jsonpatch as soon as possible.

[kdada]

@tamalsaha I can't reconcile go mod and dep.

go mod asks me to import "gomodules.xyz/jsonpatch/v2" but dep requires "gomodules.xyz/jsonpatch"

[tamalsaha]

Hmm. I am not sure how to fix this. I think it is time to drop Go dep?

[tamalsaha]

golang/dep#1963 was supposed to help but I don't know when that will happen. Given Go 1.13 is going to come out in a month (Aug 1) and Kubernetes 1.15 has go mod support, it might be time to drop Go dep support.

[kdada]

@tamalsaha I'll update this PR and use go mod only. I think we need another PR to remove dep.

[tamalsaha]

cc: @mengqiy @DirectXMan12

[mengqiy]

@kdada Can you use something like the following in Gopkg.toml to fix the issue mentioned in #499 (comment)

[[constraint]]
  name = "gomodules.xyz/jsonpatch"
  version = "v2.0.0"

[tamalsaha]

@mengqiy , I am afraid that this will not work. Because jsonpatch now uses v2, go mod vendor puts it under vendor/gomodules.xyz/jsonpatch/v2 folder. But godep has no idea of go modules. So, it will vendor it under vendor/gomodules.xyz/jsonpatch . golang/dep#1963 was supposed to fix this.

[DirectXMan12]

I'd prefer not to break dep users till we promised (v1.13 release, and probably later to be nice), but if there's no other way, we might have to cave.

@tamalsaha are you involved in the maintenance of that project? Shouldn't the import path returned by the meta tag in curl https://gomodules.xyz/jsonpatch/v2?go-get=1 return gomodules.xyz/jsonpatch/v2? If I had to guess, that's why dep is complaining.

[DirectXMan12]

need to double check

[tamalsaha]

@DirectXMan12, I am the maintainer of the jsonpatch repo. I am following the instructions here: https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher . I also looked at https://github.com/google/go-github as an example. I think I did it correctly. This is my first time releasing a v2 for a go mod project. So, if you see any issue, please let me know.

[DirectXMan12]

yep, that works. You can test by building a dep with:

diff --git a/gps/deduce.go b/gps/deduce.go
index ee097e18..3c372968 100644
--- a/gps/deduce.go
+++ b/gps/deduce.go
@@ -1003,5 +1003,8 @@ func getMetadata(ctx context.Context, path, scheme string) (string, string, stri
        if match == -1 {
                return "", "", "", errors.Errorf("go-import metadata not found")
        }
+       if imports[match].Prefix == "gomodules.xyz/jsonpatch" && path == "gomodules.xyz/jsonpatch/v2" {
+               return path, imports[match].VCS, imports[match].RepoRoot, nil
+       }
        return imports[match].Prefix, imports[match].VCS, imports[match].RepoRoot, nil
 }

[DirectXMan12]

(which'll fake out dep, which is easier to stand up than a proxy which fakes out that URL)

[DirectXMan12]

@tamalsaha I think you did the actual module correctly :-)

What's incorrect (I believe) is your webserver for gomodules.xyz that serves the special <meta> tags that tell go get how to do its thing.

[tamalsaha]

I am just using https://github.com/gomodules/govanityurls deployed on appengine.

[tamalsaha]

I have updated the https://github.com/gomodules/jsonpatch repo to use the Major subdirectory structure. So, it should work ok with both dep and go mod.

gomodules/jsonpatch#20 (comment)

[kdada]

@tamalsaha @DirectXMan12 PTAL.

[tamalsaha]

@kdada , LGTM. You may also want to add the dependency to the Gopkg.toml file as @mengqiy mentioned here #499 (comment) . dep ensure automatically picked it up as evident from the Gopkg.lock file but I would add the dependency directly in the Gopkg.toml.

[kdada]

@tamalsaha It seems I can only add the dependency with [[override]]. I think it's not necessary to add the dependency if the jsonpatch keeps backwards compatibility.

[mengqiy]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: kdada

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[DirectXMan12]

I am just using https://github.com/gomodules/govanityurls deployed on appengine.

I think that may need to be tweaked then. Switching to the major subdirectory structure also works for now :-)