Issue #615 - fixed readiness-gate Git conflicts

- previous solution broke existing functionality to copy comments between Kptfile objects
  - resulting in failing unit tests
    - fix for that brought back the Git conflicts caused by indentation, field order etc.
    - re-refactored conversion of Kptfiles to YAML
      - KubeObject-related conversion methods (KubeObject/YAML and YAML/KubeObject)
        now live out in the util package as they aren't Kptfile-specific any more

nephio-project/nephio#615

controllers/packagevariants/pkg/controllers/packagevariant/injection.go

@@ -27,6 +27,7 @@ import (
 	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
 	api "github.com/nephio-project/porch/controllers/packagevariants/api/v1alpha1"
 	kptfilev1 "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1"
+	"github.com/nephio-project/porch/pkg/util"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -125,11 +126,7 @@ func ensureConfigInjection(ctx context.Context,
 		return err
 	}
 
-	kptfileYaml, err := kptfilev1.ToYamlString(kptfile)
-	if err != nil {
-		return err
-	}
-	prr.Spec.Resources[kptfilev1.KptFileName] = kptfileYaml
+	prr.Spec.Resources[kptfilev1.KptFileName] = util.KubeObjectToYaml(kptfile)
 
 	return nil
 }

controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go

@@ -1,4 +1,4 @@
-// Copyright 2023-2024 The kpt and Nephio Authors
+// Copyright 2022, 2024 The kpt and Nephio Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -28,6 +28,7 @@ import (
 	"github.com/GoogleContainerTools/kpt-functions-sdk/go/fn"
 	kptfilev1 "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1"
 	"github.com/nephio-project/porch/pkg/kpt/kptfileutil"
+	"github.com/nephio-project/porch/pkg/util"
 
 	"golang.org/x/mod/semver"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -1088,11 +1089,7 @@ func ensureKRMFunctions(pv *api.PackageVariant,
 	}
 
 	// update kptfile
-	kptfileYaml, err := kptfilev1.ToYamlString(kptfile)
-	if err != nil {
-		return err
-	}
-	prr.Spec.Resources[kptfilev1.KptFileName] = kptfileYaml
+	prr.Spec.Resources[kptfilev1.KptFileName] = util.KubeObjectToYaml(kptfile)
 
 	return nil
 }

controllers/packagevariantsets/config/samples/pvs-v1alpha2.yaml

@@ -1,4 +1,4 @@
-# Copyright 2023 The kpt and Nephio Authors
+# Copyright 2023-2024 The kpt and Nephio Authors
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -28,7 +28,7 @@ spec:
       - foo-01
       - foo-02
       - foo-03
-      name: cluster-02
+    - name: cluster-02
     template:
       downstream:
         packageExpr: "target.package + '-' + repository.labels['env']"

func/internal/podevaluator.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The kpt and Nephio Authors
+// Copyright 2022, 2024 The kpt and Nephio Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -238,7 +238,7 @@ type imagePodAndGRPCClient struct {
 func (pcm *podCacheManager) warmupCache(podTTLConfig string) error {
 	start := time.Now()
 	defer func() {
-		klog.Infof("cache warning is completed and it took %v", time.Since(start))
+		klog.Infof("cache warming is complete after %v", time.Since(start))
 	}()
 	content, err := os.ReadFile(podTTLConfig)
 	if err != nil {

pkg/kpt/api/kptfile/v1/types.go

@@ -1,4 +1,4 @@
-// Copyright 2021 The kpt and Nephio Authors
+// Copyright 2021, 2024 The kpt and Nephio Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/GoogleContainerTools/kpt-functions-sdk/go/fn"
+	"github.com/nephio-project/porch/pkg/util"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
@@ -77,13 +78,13 @@ type KptFile struct {
 	// Info contains metadata such as license, documentation, etc.
 	Info *PackageInfo `yaml:"info,omitempty" json:"info,omitempty"`
 
+	Status *Status `yaml:"status,omitempty" json:"status,omitempty"`
+
 	// Pipeline declares the pipeline of functions.
 	Pipeline *Pipeline `yaml:"pipeline,omitempty" json:"pipeline,omitempty"`
 
 	// Inventory contains parameters for the inventory object used in apply.
 	Inventory *Inventory `yaml:"inventory,omitempty" json:"inventory,omitempty"`
-
-	Status *Status `yaml:"status,omitempty" json:"status,omitempty"`
 }
 
 func FromKubeObject(kptfileKubeObject *fn.KubeObject) (KptFile, error) {
@@ -99,16 +100,12 @@ func (file *KptFile) ToYamlString() (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return string(b), nil
-}
-
-func ToYamlString(kptfileKubeObject *fn.KubeObject) (string, error) {
-	kptfile, err := FromKubeObject(kptfileKubeObject)
+	kptfileKubeObject, err := util.YamlToKubeObject(string(b))
 	if err != nil {
 		return "", err
 	}
 
-	return kptfile.ToYamlString()
+	return kptfileKubeObject.String(), nil
 }
 
 // OriginType defines the type of origin for a package.
@@ -236,6 +233,8 @@ type PackageInfo struct {
 	// Relative slash-delimited path to the license file (e.g. LICENSE.txt)
 	LicenseFile string `yaml:"licenseFile,omitempty" json:"licenseFile,omitempty"`
 
+	ReadinessGates []ReadinessGate `yaml:"readinessGates,omitempty" json:"readinessGates,omitempty"`
+
 	// Description contains a short description of the package.
 	Description string `yaml:"description,omitempty" json:"description,omitempty"`
 
@@ -244,8 +243,6 @@ type PackageInfo struct {
 
 	// Man is the path to documentation about the package
 	Man string `yaml:"man,omitempty" json:"man,omitempty"`
-
-	ReadinessGates []ReadinessGate `yaml:"readinessGates,omitempty" json:"readinessGates,omitempty"`
 }
 
 type ReadinessGate struct {
@@ -315,6 +312,11 @@ func (p *Pipeline) IsEmpty() bool {
 // Function specifies a KRM function.
 // +kubebuilder:object:generate=true
 type Function struct {
+
+	// `Name` is used to uniquely identify the function declaration
+	// this is primarily used for merging function declaration with upstream counterparts
+	Name string `yaml:"name,omitempty" json:"name,omitempty"`
+
 	// `Image` specifies the function container image.
 	// It can either be fully qualified, e.g.:
 	//
@@ -344,10 +346,6 @@ type Function struct {
 	// `ConfigMap` is a convenient way to specify a function config of kind ConfigMap.
 	ConfigMap map[string]string `yaml:"configMap,omitempty" json:"configMap,omitempty"`
 
-	// `Name` is used to uniquely identify the function declaration
-	// this is primarily used for merging function declaration with upstream counterparts
-	Name string `yaml:"name,omitempty" json:"name,omitempty"`
-
 	// `Selectors` are used to specify resources on which the function should be executed
 	// if not specified, all resources are selected
 	Selectors []Selector `yaml:"selectors,omitempty" json:"selectors,omitempty"`
@@ -412,9 +410,9 @@ type Condition struct {
 
 	Status ConditionStatus `yaml:"status" json:"status"`
 
-	Reason string `yaml:"reason,omitempty" json:"reason,omitempty"`
-
 	Message string `yaml:"message,omitempty" json:"message,omitempty"`
+
+	Reason string `yaml:"reason,omitempty" json:"reason,omitempty"`
 }
 
 type ConditionStatus string

pkg/kpt/kptfileutil/util_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020 The kpt and Nephio Authors
+// Copyright 2020, 2024 The kpt and Nephio Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -364,8 +364,8 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
 `,
 		},
 		"additional readinessGate and condition added in upstream": {
@@ -381,8 +381,8 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
 `,
 			updated: `
 apiVersion: kpt.dev/v1
@@ -397,12 +397,12 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: bar
       status: "False"
-      reason: reason
       message: message
+      reason: reason
 `,
 			local: `
 apiVersion: kpt.dev/v1
@@ -416,8 +416,8 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
 `,
 			updateUpstream: false,
 			expected: `
@@ -433,12 +433,12 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: bar
       status: "False"
-      reason: reason
       message: message
+      reason: reason
 		`,
 		},
 		"readinessGate added removed in upstream": {
@@ -502,12 +502,12 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: bar
       status: "False"
-      reason: reason
       message: message
+      reason: reason
 `,
 			updated: `
 apiVersion: kpt.dev/v1
@@ -522,12 +522,12 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: zork
       status: "Unknown"
-      reason: reason
       message: message
+      reason: reason
 `,
 			local: `
 apiVersion: kpt.dev/v1
@@ -542,12 +542,12 @@ status:
   conditions:
     - type: xandar
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: foo
       status: "True"
-      reason: reason
       message: message  
+      reason: reason
 `,
 			updateUpstream: false,
 			expected: `
@@ -563,12 +563,12 @@ status:
   conditions:
     - type: foo
       status: "True"
-      reason: reason
       message: message
+      reason: reason
     - type: zork
       status: Unknown
-      reason: reason
       message: message
+      reason: reason
 `,
 		},
 	}

pkg/task/generictaskhandler.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 
-	sdkfn "github.com/GoogleContainerTools/kpt-functions-sdk/go/fn"
 	api "github.com/nephio-project/porch/api/porch/v1alpha1"
 	configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
 	"github.com/nephio-project/porch/internal/kpt/builtins"
@@ -528,17 +527,6 @@ func healConfig(old, new map[string]string) (map[string]string, error) {
 
 	healed := out.output.Contents
 
-	var kptfileKubeObject *sdkfn.KubeObject
-	if kptfileKubeObject, err = sdkfn.ParseKubeObject([]byte(healed[kptfile.KptFileName])); err != nil {
-		return nil, err
-	}
-
-	kptfileYaml, err := kptfile.ToYamlString(kptfileKubeObject)
-	if err != nil {
-		return nil, err
-	}
-	healed[kptfile.KptFileName] = kptfileYaml
-
 	for k, v := range extra {
 		healed[k] = v
 	}

pkg/task/kio.go

@@ -51,6 +51,9 @@ func (r *packageReader) Read() ([]*yaml.RNode, error) {
 				kioutil.PathAnnotation: k,
 			},
 			DisableUnwrapping: true,
+			// need to preserve indentation to avoid Git conflicts
+			// in written-out YAML
+			PreserveSeqIndent: true,
 		}
 		nodes, err := reader.Read()
 		if err != nil {

pkg/task/patch_test.go

@@ -26,7 +26,7 @@ import (
 	"github.com/nephio-project/porch/pkg/repository/fake"
 )
 
-func TestSomething(t *testing.T) {
+func TestGenerateKptfilePatches(t *testing.T) {
 	testCases := map[string]struct {
 		repoPkgRev   repository.PackageRevision
 		newApiPkgRev *api.PackageRevision
@@ -145,12 +145,12 @@ func TestSomething(t *testing.T) {
    conditions:
    - type: foo
      status: "True"
-+    reason: reason
 +    message: message
++    reason: reason
 +  - type: bar
 +    status: "False"
-+    reason: reason
 +    message: message
++    reason: reason
 `) + "\n",
 				PatchType: api.PatchTypePatchFile,
 			},
@@ -279,12 +279,12 @@ func TestSomething(t *testing.T) {
    conditions:
    - type: foo
      status: "True"
--    reason: reason
 -    message: message
+-    reason: reason
 -  - type: bar
 -    status: "False"
--    reason: reason
 -    message: message
+-    reason: reason
 `) + "\n",
 				PatchType: api.PatchTypePatchFile,
 			},