Skip to content
This repository has been archived by the owner on Nov 17, 2021. It is now read-only.

Rework kubecfg diff code. #230

Merged
merged 2 commits into from Dec 3, 2018
Merged

Rework kubecfg diff code. #230

merged 2 commits into from Dec 3, 2018

Conversation

mplzik
Copy link
Contributor

@mplzik mplzik commented Nov 13, 2018

The old code relied on gojsondiff library, which provides more semantic
JSON diffs. This library, however, had some long-standing issues which
have not been yet fixed, namely:

These sometimes resulted in diffs that were too noisy and/or showed
false information; this has been reported as

#229

This patch reworks kubecfg's diff code to first marshal JSON using
golang's encoding/json and then calculates the diff using
diffmatchpatch library, formatting the output as close to the
original as possible. Although this results in a more
syntactic-level diff, it might just do the job until gojsondiff
gets the above-mentioned bugs fixed.

Signed-off-by: Milan Plzik milan.plzik@ceai.io

The old code relied on gojsondiff library, which provides more semantic
JSON diffs. This library, however, had some long-standing issues which
have not been yet fixed, namely:

* yudai/gojsondiff#24
* yudai/gojsondiff#30

These sometimes resulted in diffs that were too noisy and/or showed
false information; this has been reported as

#229

This patch reworks kubecfg's diff code to first marshal JSON using
golang's encoding/json and then calculates the diff using
diffmatchpatch library, formatting the output as close to the
original as possible. Although this results in a more
syntactic-level diff, it might just do the job until gojsondiff
gets the above-mentioned bugs fixed.

Signed-off-by: Milan Plzik <milan.plzik@ceai.io>
Copy link
Contributor

@mkmik mkmik left a comment

Choose a reason for hiding this comment

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

I tried it with --diff-strategy subset and it shows "ERROR Differences found" although no differences are shown in the output. Example:

---
- live services zerozone.zerozone
+ config services zerozone.zerozone
  {
    "apiVersion": "v1",
    "kind": "Service",
    "metadata": {
      "labels": {
        "name": "foo"
      },
      "name": "foo",
      "namespace": "foo"
    },
    "spec": {
      "ports": [
        {
          "port": 8080
        }
      ],
      "selector": {
        "name": "foo"
      },
      "type": "LoadBalancer"
    }
  }
ERROR Differences found.

Master, for the same config shows:

...
-         "port": 8080
+         "port": 8080
...

pkg/kubecfg/diff.go Outdated Show resolved Hide resolved
This addresses code review comments -- the conditions in original PR
were detected incorrectly; this PR changes the condition to the case
where the whole Diff is just a single block of DiffEquals change.

Also, the latter change moves DiffLineStart to the top of file,
so that it doesn't need to be recomputed each time the formatDiff
function is called.

Signed-off-by: Milan Plzik <milan.plzik@ceai.io>
@mplzik
Copy link
Contributor Author

mplzik commented Nov 27, 2018

PTAL.

@mkmik
Copy link
Contributor

mkmik commented Nov 27, 2018

LGTM.

but sadly EMENOMAINTAINER of this repo; pinging my colleague @anguslees.

@jjo
Copy link
Contributor

jjo commented Nov 28, 2018

LGTM also, thanks @emempi -- verified it fixes false diff positives
on numeric values, and much better handles e.g. configmap diffs
with multiline valued fields.

Copy link
Contributor

@anguslees anguslees left a comment

Choose a reason for hiding this comment

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

@mkmik / @jjo: Thanks for doing the leg-work to verify the results.

Edit: @emempi .. and of course, thanks for the actual patch!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants