Initial support for fancy(er) Diff output

[Oded-B]

• Add initial support for fancy diff in diffLiveVsTargetObject function + test

Description

Things to note in new Diff format:

• apiVersion, kind and object name are always displayed.
• a short human desc: "one list entry added"
• change path, like @@ rbacBindings.security-audit-viewer-vault.subjects @@ include name of the array element: "security-audit-viewer-vault" (see old diff below)

New:

# apiVersion: commercetools.io/v1alpha1
# kind: Bar
# name: example-baz-bar
@@ rbacBindings.security-audit-viewer-vault.subjects @@
! - one list entry removed:
- - name: "vault:[email protected]"
-   kind: Group
! + one list entry added:
+   - name: "vault:[email protected]"
+     kind: Group

@@ spec.replicas @@
! ± value change
- 63
+ 42

Old:

diff live target
--- live
+++ target
@@ -21,14 +21,14 @@
     name: example-baz-bar
     namespace: gitops-demo-ns2
     resourceVersion: "2168525640"
     uid: f845fd72-d6d9-48f2-b0f2-2def6807deb8
   rbacBindings:
   - clusterRoleBindings:
     - clusterRole: view
     name: security-audit-viewer-vault
     subjects:
     - kind: Group
-      name: vault:[email protected]
+      name: vault:[email protected]
   spec:
     deploymentName: example-baz-bar
-    replicas: 63
+    replicas: 42

TODO

• can I avoid the unstructured.Unstructured > Byte > YAML3 dance?

	//  unstructured.Unstructured > Byte
	marsheledLive, _ := live.MarshalJSON()
	marsheledTarget, _ := target.MarshalJSON()

	// Byte > YAML3
	_ = yaml3.Unmarshal(marsheledLive, &liveNode)
	_ = yaml3.Unmarshal(marsheledTarget, &targetNode)

• gate behind configuration
• Document

Type of Change

• Bug Fix
• New Feature
• Breaking Change
• Refactor
• Documentation
• Other (please describe)

Checklist

• I have read the contributing guidelines
• Existing issues have been referenced (where applicable)
• I have verified this change is not present in other open pull requests
• Functionality is documented
• All code style checks pass
• New code contribution is covered by automated tests
• All new and existing tests pass

[Oded-B]

@hnnsgstfssn(and team) WDYT about this diff format?

[hnnsgstfssn]

Hey, thanks! I've notified the team to take a look.

Doing a first pass.

I'm in general favour of adding the new format. We're going to use it in some new internal tooling we're building.

The "one list entry removed/added" seems a bit excessive, but that's just me.

How would we allow configuration of the new format?
Should we completely replace the old format?

[hnnsgstfssn]

How come the indentation of the two differs? The added item has more indentation that the removed item.

[hnnsgstfssn]

This should return the error and not just log it, no? What happens below in case of error here?

[hnnsgstfssn]

Same here, what happens below in case of error here? At least it could include the error message in the returned diff string or fail altogether.

[hnnsgstfssn]

Are these guaranteed to be available? If not, then should something be displayed differently when they are empty?

[Oded-B]

This keys should exist in all k8s object(as opposed to "namespace" and such ), and the function used to get them returns empty strings in case of missing key, so the UX might not be amazing but its still "safe".
I can test for values in the formatting phase , do you think I should?

[Oded-B]

We're going to use it in some new internal tooling we're building.

Do you want to make these part of Telefonistka consumable as a lib or use telefonistka just for diffs?
I'm somewhat interested in the latter for my current company

The "one list entry removed/added" seems a bit excessive, but that's just me.

OK, let me see if there's an easy way to get rid of it

How would we allow configuration of the new format? Should we completely replace the old format?

My initial thought was to use some configuration like .argoCd.useFancyDiffFormat , but I'm not sure it makes sense to support both of them, maybe ask the team?