Merge pull request #7 from stevendborrelli/patch-ordering

Patch ordering

Author: stevendborrelli

README.md

@@ -18,7 +18,7 @@ metadata:
   annotations:
     render.crossplane.io/runtime: Development
 spec:
-  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.3.0
+  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.4.0
 ```
 
 ## What this function does

examples/conditional-rendering/functions.yaml

@@ -3,5 +3,5 @@ kind: Function
 metadata:
   name: function-conditional-patch-and-transform
 spec:
-  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.3.0
+  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.4.0
   packagePullPolicy: Always

examples/conditional-resources/composition.yaml

@@ -15,6 +15,7 @@ spec:
     input:
       apiVersion: conditional-pt.fn.crossplane.io/v1beta1
       kind: Resources
+      condition: observed.composite.resource.spec.render == true 
       resources:
          - name: blue-resource
            condition: observed.composite.resource.spec.deployment.blue == true

examples/conditional-resources/functions.yaml

@@ -3,5 +3,5 @@ kind: Function
 metadata:
   name: function-conditional-patch-and-transform
 spec:
-  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.3.0
+  package: xpkg.upbound.io/borrelli-org/function-conditional-patch-and-transform:v0.4.0
   packagePullPolicy: Always

fn.go

@@ -131,17 +131,11 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
 	}
 
 	if input.Environment != nil {
-		// Run all patches that are from the (observed) XR to the environment.
-		if err := RenderToEnvironmentPatches(env, oxr.Resource, input.Environment.Patches); err != nil {
+		// Run all patches that are from the (observed) XR to the environment or from the environment to the (desired) XR.
+		if err := RenderEnvironmentPatches(env, oxr.Resource, dxr.Resource, input.Environment.Patches); err != nil {
 			response.Fatal(rsp, errors.Wrapf(err, "cannot render ToEnvironment patches from the composite resource"))
 			return rsp, nil
 		}
-
-		// Run all patches that are from the environment to the (desired) XR.
-		if err := RenderFromEnvironmentPatches(dxr.Resource, env, input.Environment.Patches); err != nil {
-			response.Fatal(rsp, errors.Wrapf(err, "cannot render FromEnvironment patches to the composite resource"))
-			return rsp, nil
-		}
 	}
 
 	// Increment this if you emit a warning result.
@@ -228,58 +222,19 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
 			log.Debug("Found corresponding observed resource",
 				"ready", ready,
 				"name", ocd.Resource.GetName())
-
-			// TODO(negz): Should failures to patch the XR be terminal? It could
-			// indicate a required patch failed. A required patch means roughly
-			// "this patch has to succeed before you mutate the resource". This
-			// is useful to make sure we never create a composed resource in the
-			// wrong state. It's less clear how useful it is for the XR, given
-			// we'll only ever be updating it, not creating it.
-
-			// We want to patch the XR from observed composed resources, not
-			// from desired state. This is because folks will typically be
-			// patching from a field that is set once the observed resource is
-			// applied such as its status.
-			if err := RenderToCompositePatches(dxr.Resource, ocd.Resource, t.Patches); err != nil {
-				response.Warning(rsp, errors.Wrapf(err, "cannot render ToComposite patches for composed resource %q", t.Name))
-				log.Info("Cannot render ToComposite patches for composed resource", "warning", err)
-				warnings++
-			}
-
-			// TODO(negz): Same as above, but for the Environment. What does it
-			// mean for a required patch to the environment to fail? Should it
-			// be terminal?
-
-			// Run all patches that are from the (observed) composed resource to
-			// the environment.
-			if err := RenderToEnvironmentPatches(env, ocd.Resource, t.Patches); err != nil {
-				response.Warning(rsp, errors.Wrapf(err, "cannot render ToEnvironment patches for composed resource %q", t.Name))
-				log.Info("Cannot render ToEnvironment patches for composed resource", "warning", err)
-				warnings++
-			}
 		}
 
-		// If either of the below renderings return an error, most likely a
-		// required FromComposite or FromEnvironment patch failed. A required
-		// patch means roughly "this patch has to succeed before you mutate the
-		// resource." This is useful to make sure we never create a composed
-		// resource in the wrong state. To that end, we don't want to add this
-		// resource to our accumulated desired state.
-		if err := RenderFromCompositePatches(dcd.Resource, oxr.Resource, t.Patches); err != nil {
-			response.Warning(rsp, errors.Wrapf(err, "cannot render FromComposite patches for composed resource %q", t.Name))
-			log.Info("Cannot render FromComposite patches for composed resource", "warning", err)
+		errs, store := RenderComposedPatches(ocd.Resource, dcd.Resource, oxr.Resource, dxr.Resource, env, t.Patches)
+		for _, err := range errs {
+			response.Warning(rsp, errors.Wrapf(err, "cannot render patches for composed resource %q", t.Name))
+			log.Info("Cannot render patches for composed resource", "warning", err)
 			warnings++
-			continue
-		}
-		if err := RenderFromEnvironmentPatches(dcd.Resource, env, t.Patches); err != nil {
-			response.Warning(rsp, errors.Wrapf(err, "cannot render FromEnvironment patches for composed resource %q", t.Name))
-			log.Info("Cannot render FromEnvironment patches for composed resource", "warning", err)
-			warnings++
-			continue
 		}
 
-		// Add or replace our desired resource.
-		desired[resource.Name(t.Name)] = dcd
+		if store {
+			// Add or replace our desired resource.
+			desired[resource.Name(t.Name)] = dcd
+		}
 	}
 
 	if err := response.SetDesiredCompositeResource(rsp, dxr); err != nil {