Repository: skupperproject/skupper
Pull request #1607 (2024-08-21 13:28:13)

[v2] Generated YAML has redundant fields
**Description of the issue**

```
$ skupper site create west -o yaml
apiVersion: skupper.io/v1alpha1
kind: Site
metadata:
  creationTimestamp: null       # ??
  name: west
  namespace: hw
spec:
  linkAccess: none              # ??
  settings:                     # ??
    name: west
status: {}
```
- `creationTimestamp` should not be there.
- `linkAccess` is explicitly setting a value that is already the default. 
- the `settings` block with the name of the site it is not necessary yet.

**Description of the solution**

Even though the kubernetes `creationTimestamp` type has the `omitempty` json tag, after encoding the `Site` custom resource (or any other resource with kubernetes metadata) to json/yaml, its value is `null` when not initialised.

For that reason, the encode function implements the function `omitEmptyValues` that check if the data contains zero values and discards them if necessary.

Besides that, the CLI will not assign the value `none` to the `linkAccessType`, and the `settings` field will not be used.



Patch:
diff --git a/internal/cmd/skupper/site/kube/site_create.go b/internal/cmd/skupper/site/kube/site_create.go
index 24fa721b..b4817290 100644
--- a/internal/cmd/skupper/site/kube/site_create.go
+++ b/internal/cmd/skupper/site/kube/site_create.go
@@ -6,16 +6,14 @@ package kube
 import (
 	"context"
 	"fmt"
-	"k8s.io/client-go/kubernetes"
-
 	"github.com/skupperproject/skupper/internal/cmd/skupper/utils"
 	"github.com/skupperproject/skupper/internal/kube/client"
 	"github.com/skupperproject/skupper/pkg/apis/skupper/v1alpha1"
 	skupperv1alpha1 "github.com/skupperproject/skupper/pkg/generated/client/clientset/versioned/typed/skupper/v1alpha1"
-	"github.com/skupperproject/skupper/pkg/site"
 	"github.com/skupperproject/skupper/pkg/utils/validator"
 	"github.com/spf13/cobra"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
 )
 
 var (
@@ -36,7 +34,6 @@ type CmdSiteCreate struct {
 	KubeClient         kubernetes.Interface
 	CobraCmd           cobra.Command
 	flags              CreateFlags
-	options            map[string]string
 	siteName           string
 	serviceAccountName string
 	Namespace          string
@@ -46,8 +43,7 @@ type CmdSiteCreate struct {
 
 func NewCmdSiteCreate() *CmdSiteCreate {
 
-	options := make(map[string]string)
-	skupperCmd := CmdSiteCreate{options: options, flags: CreateFlags{}}
+	skupperCmd := CmdSiteCreate{flags: CreateFlags{}}
 
 	cmd := cobra.Command{
 		Use:    "create <name>",
@@ -152,15 +148,8 @@ func (cmd *CmdSiteCreate) InputToOptions() {
 		} else {
 			cmd.linkAccessType = cmd.flags.linkAccessType
 		}
-	} else {
-		cmd.linkAccessType = "none"
 	}
 
-	options := make(map[string]string)
-	options[site.SiteConfigNameKey] = cmd.siteName
-
-	cmd.options = options
-
 	cmd.output = cmd.flags.output
 
 }
@@ -177,7 +166,6 @@ func (cmd *CmdSiteCreate) Run() error {
 			Namespace: cmd.Namespace,
 		},
 		Spec: v1alpha1.SiteSpec{
-			Settings:       cmd.options,
 			ServiceAccount: cmd.serviceAccountName,
 			LinkAccess:     cmd.linkAccessType,
 		},
diff --git a/internal/cmd/skupper/utils/encode.go b/internal/cmd/skupper/utils/encode.go
index fd0554cd..aaf70a85 100644
--- a/internal/cmd/skupper/utils/encode.go
+++ b/internal/cmd/skupper/utils/encode.go
@@ -3,6 +3,7 @@ package utils
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"sigs.k8s.io/yaml"
 )
 
@@ -13,12 +14,75 @@ func Encode(outputType string, resource interface{}) (string, error) {
 
 	switch outputType {
 	case "json":
-		result, err = json.MarshalIndent(resource, "", "  ")
+		{
+			initialMap := make(map[string]interface{})
+			jsonData, err := json.MarshalIndent(resource, "", "  ")
+			if err != nil {
+				return "", err
+			}
+
+			err = json.Unmarshal(jsonData, &initialMap)
+			if err != nil {
+				return "", err
+			}
+
+			cleanedMap := omitEmptyValues(initialMap)
+			result, err = json.MarshalIndent(cleanedMap, "", "  ")
+		}
 	case "yaml":
-		result, err = yaml.Marshal(resource)
+		{
+			initialMap := make(map[string]interface{})
+			yamlData, err := yaml.Marshal(resource)
+			if err != nil {
+				return "", err
+			}
+
+			err = yaml.Unmarshal(yamlData, &initialMap)
+			if err != nil {
+				return "", err
+			}
+
+			cleanedMap := omitEmptyValues(initialMap)
+
+			result, err = yaml.Marshal(cleanedMap)
+		}
+
 	default:
 		return "", fmt.Errorf("format %s not supported", outputType)
 	}
 
 	return string(result), err
 }
+
+func omitEmptyValues(data map[string]interface{}) map[string]interface{} {
+	cleaned := make(map[string]interface{})
+	for k, v := range data {
+		if !isZeroValue(v) {
+			if reflect.TypeOf(v).Kind() == reflect.Map {
+				if vm, ok := v.(map[string]interface{}); ok {
+					v = omitEmptyValues(vm)
+				}
+			}
+			cleaned[k] = v
+		}
+	}
+	return cleaned
+}
+
+func isZeroValue(x interface{}) bool {
+	if x == nil {
+		return true
+	}
+
+	v := reflect.ValueOf(x)
+	switch v.Kind() {
+	case reflect.Map, reflect.Slice, reflect.Array:
+		return v.Len() == 0
+	case reflect.Struct:
+		z := reflect.New(v.Type()).Elem().Interface()
+		return reflect.DeepEqual(x, z)
+	default:
+		z := reflect.Zero(v.Type()).Interface()
+		return reflect.DeepEqual(x, z)
+	}
+}


Test patch:
diff --git a/internal/cmd/skupper/site/kube/site_create_test.go b/internal/cmd/skupper/site/kube/site_create_test.go
index 9064b591..17d2a10f 100644
--- a/internal/cmd/skupper/site/kube/site_create_test.go
+++ b/internal/cmd/skupper/site/kube/site_create_test.go
@@ -168,60 +168,44 @@ func TestCmdSiteCreate_InputToOptions(t *testing.T) {
 		name               string
 		args               []string
 		flags              CreateFlags
-		expectedSettings   map[string]string
 		expectedLinkAccess string
 		expectedOutput     string
 	}
 
 	testTable := []test{
 		{
-			name:  "options without link access enabled",
-			args:  []string{"my-site"},
-			flags: CreateFlags{},
-			expectedSettings: map[string]string{
-				"name": "my-site",
-			},
-			expectedLinkAccess: "none",
+			name:               "options without link access enabled",
+			args:               []string{"my-site"},
+			flags:              CreateFlags{},
+			expectedLinkAccess: "",
 			expectedOutput:     "",
 		},
 		{
-			name:  "options with link access enabled but using a type by default and link access host specified",
-			args:  []string{"my-site"},
-			flags: CreateFlags{enableLinkAccess: true},
-			expectedSettings: map[string]string{
-				"name": "my-site",
-			},
+			name:               "options with link access enabled but using a type by default",
+			args:               []string{"my-site"},
+			flags:              CreateFlags{enableLinkAccess: true, linkAccessType: "loadbalancer"},
 			expectedLinkAccess: "loadbalancer",
 			expectedOutput:     "",
 		},
 		{
-			name:  "options with link access enabled using the nodeport type",
-			args:  []string{"my-site"},
-			flags: CreateFlags{enableLinkAccess: true, linkAccessType: "nodeport"},
-			expectedSettings: map[string]string{
-				"name": "my-site",
-			},
+			name:               "options with link access enabled using the nodeport type",
+			args:               []string{"my-site"},
+			flags:              CreateFlags{enableLinkAccess: true, linkAccessType: "nodeport"},
 			expectedLinkAccess: "nodeport",
 			expectedOutput:     "",
 		},
 		{
-			name:  "options with link access options not well specified",
-			args:  []string{"my-site"},
-			flags: CreateFlags{enableLinkAccess: false, linkAccessType: "nodeport"},
-			expectedSettings: map[string]string{
-				"name": "my-site",
-			},
-			expectedLinkAccess: "none",
+			name:               "options with link access options not well specified",
+			args:               []string{"my-site"},
+			flags:              CreateFlags{enableLinkAccess: false, linkAccessType: "nodeport"},
+			expectedLinkAccess: "",
 			expectedOutput:     "",
 		},
 		{
-			name:  "options output type",
-			args:  []string{"my-site"},
-			flags: CreateFlags{enableLinkAccess: false, linkAccessType: "nodeport", output: "yaml"},
-			expectedSettings: map[string]string{
-				"name": "my-site",
-			},
-			expectedLinkAccess: "none",
+			name:               "options output type",
+			args:               []string{"my-site"},
+			flags:              CreateFlags{enableLinkAccess: false, linkAccessType: "nodeport", output: "yaml"},
+			expectedLinkAccess: "",
 			expectedOutput:     "yaml",
 		},
 	}
@@ -235,9 +219,8 @@ func TestCmdSiteCreate_InputToOptions(t *testing.T) {
 
 			cmd.InputToOptions()
 
-			assert.DeepEqual(t, cmd.options, test.expectedSettings)
-
 			assert.Check(t, cmd.output == test.expectedOutput)
+			assert.Check(t, cmd.linkAccessType == test.expectedLinkAccess)
 		})
 	}
 }
@@ -321,7 +304,6 @@ func TestCmdSiteCreate_Run(t *testing.T) {
 
 		command.siteName = test.siteName
 		command.serviceAccountName = test.serviceAccountName
-		command.options = test.options
 		command.output = test.output
 
 		t.Run(test.name, func(t *testing.T) {
diff --git a/internal/cmd/skupper/utils/encode_test.go b/internal/cmd/skupper/utils/encode_test.go
index 0d6291a3..e85ee290 100644
--- a/internal/cmd/skupper/utils/encode_test.go
+++ b/internal/cmd/skupper/utils/encode_test.go
@@ -27,17 +27,15 @@ func Test_Marshal(t *testing.T) {
 			},
 		},
 			`{
-  "kind": "Site",
   "apiVersion": "skupper.io/v1alpha1",
+  "kind": "Site",
   "metadata": {
     "name": "my-site",
-    "namespace": "test",
-    "creationTimestamp": null
+    "namespace": "test"
   },
   "spec": {
     "linkAccess": "default"
-  },
-  "status": {}
+  }
 }`, false},
 		{"yaml", v1alpha1.Site{
 			TypeMeta: v1.TypeMeta{
@@ -55,12 +53,10 @@ func Test_Marshal(t *testing.T) {
 			`apiVersion: skupper.io/v1alpha1
 kind: Site
 metadata:
-  creationTimestamp: null
   name: my-site
   namespace: test
 spec:
   linkAccess: default
-status: {}
 `,
 			false},
 		{"unsupported", v1alpha1.Site{