Merge pull request #275 from justinsb/take_objects_not_strings

Change ApplierOptions to take Objects, not a manifest.

Author: k8s-ci-robot

pkg/patterns/declarative/metrics_test.go

@@ -525,7 +525,7 @@ func TestAddIfNotPresent(t *testing.T) {
 				if st.actions[i] == "Create" || st.actions[i] == "Update" {
 					var options applier.ApplierOptions
 					options.Namespace = st.defaultNamespace
-					options.Manifest = yobj
+					options.Objects = objList
 					options.RESTMapper = restMapper
 					options.RESTConfig = restConfig
 					applier := applier.NewApplySetApplier(metav1.PatchOptions{FieldManager: "kdp-test"})

pkg/patterns/declarative/pkg/applier/applylib.go

@@ -9,7 +9,6 @@ import (
 	"k8s.io/client-go/dynamic"
 
 	"sigs.k8s.io/kubebuilder-declarative-pattern/applylib/applyset"
-	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 type ApplySetApplier struct {
@@ -23,10 +22,6 @@ func NewApplySetApplier(patchOptions metav1.PatchOptions) *ApplySetApplier {
 }
 
 func (a *ApplySetApplier) Apply(ctx context.Context, opt ApplierOptions) error {
-	objects, err := manifest.ParseObjects(ctx, opt.Manifest)
-	if err != nil {
-		return fmt.Errorf("error parsing manifest: %w", err)
-	}
 
 	patchOptions := a.patchOptions
 
@@ -61,7 +56,7 @@ func (a *ApplySetApplier) Apply(ctx context.Context, opt ApplierOptions) error {
 
 	// Populate the namespace on any namespace-scoped objects
 	if opt.Namespace != "" {
-		for _, obj := range objects.Items {
+		for _, obj := range opt.Objects {
 			gvk := obj.GroupVersionKind()
 			restMapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 			if err != nil {
@@ -81,7 +76,7 @@ func (a *ApplySetApplier) Apply(ctx context.Context, opt ApplierOptions) error {
 	}
 
 	var applyableObjects []applyset.ApplyableObject
-	for _, obj := range objects.Items {
+	for _, obj := range opt.Objects {
 		applyableObject := obj.UnstructuredObject()
 		applyableObjects = append(applyableObjects, applyableObject)
 	}

pkg/patterns/declarative/pkg/applier/applylib_test.go

@@ -14,6 +14,7 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 	controllerrestmapper "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/restmapper"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/test/httprecorder"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/test/testharness"
@@ -69,15 +70,20 @@ func runApplierGoldenTests(t *testing.T, testDir string, interceptHTTPServer boo
 				t.Fatalf("error precreating objects: %v", err)
 			}
 		}
-		manifest := string(h.MustReadFile(filepath.Join(testdir, "manifest.yaml")))
+		p := filepath.Join(testdir, "manifest.yaml")
+		manifestYAML := string(h.MustReadFile(p))
+		objects, err := manifest.ParseObjects(ctx, manifestYAML)
+		if err != nil {
+			t.Errorf("error parsing manifest %q: %v", p, err)
+		}
 
 		restMapper, err := controllerrestmapper.NewControllerRESTMapper(restConfig)
 		if err != nil {
 			t.Fatalf("error from controllerrestmapper.NewControllerRESTMapper: %v", err)
 		}
 
 		options := ApplierOptions{
-			Manifest:   manifest,
+			Objects:    objects.GetItems(),
 			RESTConfig: restConfig,
 			RESTMapper: restMapper,
 		}

pkg/patterns/declarative/pkg/applier/direct.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/kubectl/pkg/cmd/apply"
 	cmdDelete "k8s.io/kubectl/pkg/cmd/delete"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 type DirectApplier struct {
@@ -68,12 +69,18 @@ func NewDirectApplier() Applier {
 }
 
 func (d *DirectApplier) Apply(ctx context.Context, opt ApplierOptions) error {
+	objects := manifest.Objects{Items: opt.Objects}
+	manifestStr, err := objects.JSONManifest()
+	if err != nil {
+		return fmt.Errorf("error creating JSON manifest: %w", err)
+	}
+
 	ioStreams := genericclioptions.IOStreams{
 		In:     os.Stdin,
 		Out:    os.Stdout,
 		ErrOut: os.Stderr,
 	}
-	ioReader := strings.NewReader(opt.Manifest)
+	ioReader := strings.NewReader(manifestStr)
 
 	b := d.inner.NewBuilder(opt)
 	f := d.inner.NewFactory(opt)

pkg/patterns/declarative/pkg/applier/direct_test.go

@@ -31,6 +31,7 @@ import (
 	kubectltesting "k8s.io/kubectl/pkg/cmd/testing"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 	"k8s.io/kubectl/pkg/scheme"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 type directApplierTestSite struct {
@@ -183,18 +184,24 @@ metadata:
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			ctx := context.TODO()
+
 			d := &directApplierTestSite{}
 			testApplier := newDirectApplierTest(d)
 
+			objects, err := manifest.ParseObjects(ctx, test.manifest)
+			if err != nil {
+				t.Errorf("error parsing manifest: %v", err)
+			}
+
 			opts := ApplierOptions{
 				Namespace: test.namespace,
-				Manifest:  test.manifest,
+				Objects:   objects.GetItems(),
 				Validate:  test.validate,
 				ExtraArgs: test.args,
 			}
 
-			err := testApplier.Apply(context.Background(), opts)
-			if err != nil {
+			if err := testApplier.Apply(ctx, opts); err != nil {
 				t.Errorf("unexpected error on call Apply: %v", err)
 			}
 

pkg/patterns/declarative/pkg/applier/exec.go

@@ -31,6 +31,7 @@ import (
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 // New creates a Client that runs kubectl avaliable on the path
@@ -93,6 +94,12 @@ func buildKubeconfig(restConfig *rest.Config) ([]byte, error) {
 func (c *ExecKubectl) Apply(ctx context.Context, opt ApplierOptions) error {
 	log := log.FromContext(ctx)
 
+	objects := manifest.Objects{Items: opt.Objects}
+	manifestStr, err := objects.JSONManifest()
+	if err != nil {
+		return fmt.Errorf("error creating JSON manifest: %w", err)
+	}
+
 	log.Info("applying manifest")
 
 	args := []string{"apply"}
@@ -142,7 +149,7 @@ func (c *ExecKubectl) Apply(ctx context.Context, opt ApplierOptions) error {
 	log.Info("applying manifest with kubectl %s", strings.Join(args, " "))
 
 	cmd := exec.Command("kubectl", args...)
-	cmd.Stdin = strings.NewReader(opt.Manifest)
+	cmd.Stdin = strings.NewReader(manifestStr)
 
 	var stdout bytes.Buffer
 	var stderr bytes.Buffer
@@ -151,10 +158,9 @@ func (c *ExecKubectl) Apply(ctx context.Context, opt ApplierOptions) error {
 
 	log.WithValues("command", "kubectl").WithValues("args", args).Info("executing kubectl")
 
-	err := c.cmdSite.Run(cmd)
-	if err != nil {
+	if err := c.cmdSite.Run(cmd); err != nil {
 		log.WithValues("stdout", stdout.String()).WithValues("stderr", stderr.String()).Error(err, "error from running kubectl apply")
-		log.Info(fmt.Sprintf("manifest:\n%v", opt.Manifest))
+		log.Info(fmt.Sprintf("manifest:\n%v", manifestStr))
 		return fmt.Errorf("error from running kubectl apply: %v", err)
 	}
 

pkg/patterns/declarative/pkg/applier/exec_test.go

@@ -22,7 +22,11 @@ import (
 	"io"
 	"os/exec"
 	"reflect"
+	"strings"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 // collector is a commandSite implementation that stubs cmd.Run() calls for tests
@@ -37,59 +41,80 @@ func (s *collector) Run(c *exec.Cmd) error {
 }
 
 func TestKubectlApply(t *testing.T) {
+	configMapYAML := `
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: foo
+`
+	configMapJSON := `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"foo"}}`
+
 	tests := []struct {
-		name       string
-		namespace  string
-		manifest   string
-		validate   bool
-		args       []string
-		err        error
-		expectArgs []string
+		name        string
+		namespace   string
+		manifest    string
+		validate    bool
+		args        []string
+		err         error
+		expectStdin string
+		expectArgs  []string
 	}{
 		{
-			name:       "manifest",
-			namespace:  "",
-			manifest:   "foo",
-			expectArgs: []string{"kubectl", "apply", "--validate=false", "-f", "-"},
+			name:        "manifest",
+			namespace:   "",
+			manifest:    configMapYAML,
+			expectStdin: configMapJSON,
+			expectArgs:  []string{"kubectl", "apply", "--validate=false", "-f", "-"},
 		},
 		{
-			name:       "manifest with apply",
-			namespace:  "kube-system",
-			manifest:   "heynow",
-			expectArgs: []string{"kubectl", "apply", "-n", "kube-system", "--validate=false", "-f", "-"},
+			name:        "manifest with apply",
+			namespace:   "kube-system",
+			manifest:    configMapYAML,
+			expectStdin: configMapJSON,
+			expectArgs:  []string{"kubectl", "apply", "-n", "kube-system", "--validate=false", "-f", "-"},
 		},
 		{
-			name:       "manifest with validate",
-			namespace:  "",
-			manifest:   "foo",
-			validate:   true,
-			expectArgs: []string{"kubectl", "apply", "--validate=true", "-f", "-"},
+			name:        "manifest with validate",
+			namespace:   "",
+			manifest:    configMapYAML,
+			expectStdin: configMapJSON,
+			validate:    true,
+			expectArgs:  []string{"kubectl", "apply", "--validate=true", "-f", "-"},
 		},
 		{
 			name:       "error propagation",
 			expectArgs: []string{"kubectl", "apply", "--validate=false", "-f", "-"},
 			err:        errors.New("error"),
 		},
 		{
-			name:       "manifest with prune",
-			namespace:  "kube-system",
-			manifest:   "heynow",
-			args:       []string{"--prune=true", "--prune-whitelist=hello-world"},
-			expectArgs: []string{"kubectl", "apply", "-n", "kube-system", "--validate=false", "--prune=true", "--prune-whitelist=hello-world", "-f", "-"},
+			name:        "manifest with prune",
+			namespace:   "kube-system",
+			manifest:    configMapYAML,
+			expectStdin: configMapJSON,
+			args:        []string{"--prune=true", "--prune-whitelist=hello-world"},
+			expectArgs:  []string{"kubectl", "apply", "-n", "kube-system", "--validate=false", "--prune=true", "--prune-whitelist=hello-world", "-f", "-"},
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			ctx := context.TODO()
+
 			cs := collector{Error: test.err}
 			kubectl := &ExecKubectl{cmdSite: &cs}
+
+			objects, err := manifest.ParseObjects(ctx, test.manifest)
+			if err != nil {
+				t.Fatalf("error parsing manifest: %v", err)
+			}
+
 			opts := ApplierOptions{
 				Namespace: test.namespace,
-				Manifest:  test.manifest,
+				Objects:   objects.GetItems(),
 				Validate:  test.validate,
 				ExtraArgs: test.args,
 			}
-			err := kubectl.Apply(context.Background(), opts)
+			err = kubectl.Apply(ctx, opts)
 
 			if test.err != nil && err == nil {
 				t.Error("expected error to occur")
@@ -110,8 +135,9 @@ func TestKubectlApply(t *testing.T) {
 			if err != nil {
 				t.Fatalf("error reading manifest from stdin: %v", err)
 			}
-			if stdin := string(stdinBytes); stdin != test.manifest {
-				t.Errorf("manifest mismatch, expected: %v, got: %v", test.manifest, stdin)
+			if got, want := strings.TrimSpace(string(stdinBytes)), test.expectStdin; got != want {
+				t.Errorf("manifest mismatch: got: %v, want: %v", got, want)
+				t.Errorf("diff=%v", cmp.Diff(got, want))
 			}
 		})
 	}

pkg/patterns/declarative/pkg/applier/type.go

@@ -6,14 +6,15 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/rest"
+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 type Applier interface {
 	Apply(ctx context.Context, options ApplierOptions) error
 }
 
 type ApplierOptions struct {
-	Manifest string
+	Objects []*manifest.Object
 
 	RESTConfig *rest.Config
 	RESTMapper meta.RESTMapper

pkg/patterns/declarative/reconciler.go

@@ -255,15 +255,6 @@ func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedN
 	}
 	objects.Items = newItems
 
-	var manifestStr string
-
-	m, err := objects.JSONManifest()
-	if err != nil {
-		log.Error(err, "creating final manifest")
-		return objects, fmt.Errorf("error creating manifest: %v", err)
-	}
-	manifestStr = m
-
 	extraArgs := []string{}
 
 	// allow user disable prune in CR
@@ -306,7 +297,7 @@ func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedN
 		RESTConfig: r.config,
 		RESTMapper: r.restMapper,
 		Namespace:  ns,
-		Manifest:   manifestStr,
+		Objects:    objects.GetItems(),
 		Validate:   r.options.validate,
 		ExtraArgs:  extraArgs,
 		Force:      true,