B: properly map version_from within across substep

previously, we didn't properly handle version_from in the across
substep. we would generate a template for the substep with dummy subplan
IDs, and when we render the template for each substep, we set each of
the subplan IDs to be unique value.

this however didn't take into account version_from for the GetPlan. for
instance, a `put` step will generate a PutPlan and a GetPlan, where the
`get` uses the `version_from` the put. since we weren't properly mapping
the version_from to the generated unique subplan ID, we weren't able to
use a put step as an across substep.

since this is such a subtle thing, and is very easy to miss, I added a
unit test that walks the atc.Plan type and verifies that any fields
containing an atc.Plan must be properly handled. a field is considered
"handled" if it's explicitly added to a whitelist of handled fields -
any other fields will be considered unhandled

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

atc/builds/planner.go

@@ -239,12 +239,6 @@ func (visitor *planVisitor) VisitAcross(step *atc.AcrossStep) error {
 		return err
 	}
 
-	// The plan is simply used as a template for generating the substeps
-	// dynamically, so it should be clear that the IDs aren't valid.
-	visitor.plan.Each(func(p *atc.Plan) {
-		p.ID = "ACROSS_SUBSTEP_TEMPLATE"
-	})
-
 	template, err := json.Marshal(visitor.plan)
 	if err != nil {
 		return err

atc/builds/planner_test.go

@@ -570,7 +570,7 @@ var factoryTests = []PlannerTest{
 						"max_in_flight": 1
 					}
 				],
-				"substep_template": "{\"id\":\"ACROSS_SUBSTEP_TEMPLATE\",\"load_var\":{\"name\":\"some-var\",\"file\":\"some-file\"}}"
+				"substep_template": "{\"id\":\"1\",\"load_var\":{\"name\":\"some-var\",\"file\":\"some-file\"}}"
 			}
 		}`,
 	},

atc/engine/build_step_delegate.go

@@ -373,11 +373,29 @@ func (delegate *buildStepDelegate) ConstructAcrossSubsteps(templateBytes []byte,
 		if err := yaml.Unmarshal(interpolatedBytes, &subPlan); err != nil {
 			return nil, fmt.Errorf("invalid template bytes: %w", err)
 		}
+
+		// Maps from the original subplan ID generated by the planner to the
+		// translated ID unique to the substep iteration.
+		mappedSubplanIDs := map[atc.PlanID]atc.PlanID{}
 		planIDCounter := 0
 		subPlan.Each(func(p *atc.Plan) {
-			p.ID = atc.PlanID(fmt.Sprintf("%s/%d/%d", delegate.planID, i, planIDCounter))
+			mappedID := atc.PlanID(fmt.Sprintf("%s/%d/%d", delegate.planID, i, planIDCounter))
+			mappedSubplanIDs[p.ID] = mappedID
+			p.ID = mappedID
 			planIDCounter++
 		})
+
+		subPlan.Each(func(p *atc.Plan) {
+			// Ensure VersionFrom is mapped to the correct subplan within the
+			// substep. Note that the VersionFrom plan ID can theoretically
+			// reside outside of the substep, in which case no mapping is
+			// necessary.
+			if p.Get != nil && p.Get.VersionFrom != nil {
+				if mappedID, ok := mappedSubplanIDs[*p.Get.VersionFrom]; ok {
+					p.Get.VersionFrom = &mappedID
+				}
+			}
+		})
 		substeps[i] = atc.VarScopedPlan{
 			Step:   subPlan,
 			Values: values,

atc/engine/build_step_delegate_test.go

@@ -4,7 +4,10 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
+	"reflect"
+	"strings"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -556,25 +559,35 @@ var _ = Describe("BuildStepDelegate", func() {
 	})
 
 	Describe("ConstructAcrossSubsteps", func() {
+		planIDPtr := func(p atc.PlanID) *atc.PlanID {
+			return &p
+		}
+
 		It("constructs the across substeps and emits them as a build event", func() {
 			template := []byte(`{
-				"id": "ACROSS_SUBSTEP_TEMPLATE",
-				"do": [
-					{
-						"id": "ACROSS_SUBSTEP_TEMPLATE",
-						"task": {
-							"name": "foo",
+				"id": "on-success-id",
+				"on_success": {
+					"step": {
+						"id": "put-id",
+						"put": {
+							"name": "((.:v1))",
+							"type": "some-type",
 							"params": {
-								"p1": "((.:v1))",
-								"p2": "howdy ((.:v2))"
-							},
-							"input_mapping": {
-								"blah": "((.:v3))",
+								"p1": "((.:v2))",
+								"p2": "howdy ((.:v3))",
 								"untouched": "((v1))"
 							}
 						}
+					},
+					"on_success": {
+						"id": "get-id",
+						"get": {
+							"name": "((.:v1))",
+							"type": "some-type",
+							"version_from": "put-id"
+						}
 					}
-				]
+				}
 			}`)
 			substeps, err := delegate.ConstructAcrossSubsteps(template, []atc.AcrossVar{
 				{Var: "v1"},
@@ -591,13 +604,25 @@ var _ = Describe("BuildStepDelegate", func() {
 					Values: []interface{}{"a1", "b1", "c1"},
 					Step: atc.Plan{
 						ID: "some-plan-id/0/0",
-						Do: &atc.DoPlan{
-							{
+						OnSuccess: &atc.OnSuccessPlan{
+							Step: atc.Plan{
 								ID: "some-plan-id/0/1",
-								Task: &atc.TaskPlan{
-									Name:         "foo",
-									Params:       atc.TaskEnv{"p1": "a1", "p2": "howdy b1"},
-									InputMapping: map[string]string{"blah": "c1", "untouched": "((v1))"},
+								Put: &atc.PutPlan{
+									Name: "a1",
+									Type: "some-type",
+									Params: atc.Params{
+										"p1":        "b1",
+										"p2":        "howdy c1",
+										"untouched": "((v1))",
+									},
+								},
+							},
+							Next: atc.Plan{
+								ID: "some-plan-id/0/2",
+								Get: &atc.GetPlan{
+									Name:        "a1",
+									Type:        "some-type",
+									VersionFrom: planIDPtr("some-plan-id/0/1"),
 								},
 							},
 						},
@@ -607,13 +632,25 @@ var _ = Describe("BuildStepDelegate", func() {
 					Values: []interface{}{"a1", "b1", "c2"},
 					Step: atc.Plan{
 						ID: "some-plan-id/1/0",
-						Do: &atc.DoPlan{
-							{
+						OnSuccess: &atc.OnSuccessPlan{
+							Step: atc.Plan{
 								ID: "some-plan-id/1/1",
-								Task: &atc.TaskPlan{
-									Name:         "foo",
-									Params:       atc.TaskEnv{"p1": "a1", "p2": "howdy b1"},
-									InputMapping: map[string]string{"blah": "c2", "untouched": "((v1))"},
+								Put: &atc.PutPlan{
+									Name: "a1",
+									Type: "some-type",
+									Params: atc.Params{
+										"p1":        "b1",
+										"p2":        "howdy c2",
+										"untouched": "((v1))",
+									},
+								},
+							},
+							Next: atc.Plan{
+								ID: "some-plan-id/1/2",
+								Get: &atc.GetPlan{
+									Name:        "a1",
+									Type:        "some-type",
+									VersionFrom: planIDPtr("some-plan-id/1/1"),
 								},
 							},
 						},
@@ -637,6 +674,113 @@ var _ = Describe("BuildStepDelegate", func() {
 				},
 			}))
 		})
+
+		It("doesn't transform Get.VersionFrom if the referenced PlanID isn't a part of the substep", func() {
+			template := []byte(`{
+				"id": "get-id",
+				"get": {
+					"name": "((.:v1))",
+					"type": "some-type",
+					"version_from": "external-id"
+				}
+			}`)
+			substeps, err := delegate.ConstructAcrossSubsteps(template, []atc.AcrossVar{
+				{Var: "v1"},
+			}, [][]interface{}{
+				{"a1"},
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			expectedSubstepPlans := []atc.VarScopedPlan{
+				{
+					Values: []interface{}{"a1"},
+					Step: atc.Plan{
+						ID: "some-plan-id/0/0",
+						Get: &atc.GetPlan{
+							Name:        "a1",
+							Type:        "some-type",
+							VersionFrom: planIDPtr("external-id"),
+						},
+					},
+				},
+			}
+
+			By("interpolating the var values into the substep plans")
+			Expect(substeps).To(Equal(expectedSubstepPlans))
+
+			By("emitting the public plans as a build event")
+			Expect(fakeBuild.SaveEventCallCount()).To(Equal(1))
+			Expect(fakeBuild.SaveEventArgsForCall(0)).To(Equal(event.AcrossSubsteps{
+				Time: now.Unix(),
+				Substeps: []*json.RawMessage{
+					expectedSubstepPlans[0].Public(),
+				},
+				Origin: event.Origin{
+					ID: "some-plan-id",
+				},
+			}))
+		})
+
+		It("handles all PlanID fields in the atc.Plan", func() {
+			// handledFields are the fields of type PlanID that are properly
+			// handled in ConstructAcrossSubsteps (i.e. that get mapped to the
+			// appropriate plan ID within the substep). If a new PlanID field
+			// is added to atc.Plan or one of its subtypes, it must be properly
+			// handled and added to this list.
+			handledFields := []string{"ID", "Get.VersionFrom"}
+
+			isHandled := func(field string) bool {
+				for _, f := range handledFields {
+					if f == field {
+						return true
+					}
+				}
+				return false
+			}
+
+			var dereference func(reflect.Type) reflect.Type
+			dereference = func(rt reflect.Type) reflect.Type {
+				switch rt.Kind() {
+				case reflect.Ptr, reflect.Array, reflect.Slice:
+					return dereference(rt.Elem())
+				default:
+					return rt
+				}
+			}
+
+			planIDType := reflect.TypeOf(atc.PlanID(""))
+
+			seen := map[reflect.Type]bool{}
+			var walk func([]string, reflect.Type)
+			walk = func(paths []string, rt reflect.Type) {
+				rt = dereference(rt)
+
+				fieldPath := strings.Join(paths, ".")
+				if rt == planIDType && !isHandled(fieldPath) {
+					Fail(fmt.Sprintf("ConstructAcrossSubsteps does not handle PlanID field %q", fieldPath))
+				}
+
+				// Avoid recursing infinitely since atc.Plan is a recursive type
+				if seen[rt] {
+					return
+				}
+				seen[rt] = true
+
+				if rt.Kind() == reflect.Map {
+					walk(append(paths, "key"), rt.Key())
+					walk(append(paths, "value"), rt.Elem())
+				}
+
+				if rt.Kind() == reflect.Struct {
+					for i := 0; i < rt.NumField(); i++ {
+						field := rt.Field(i)
+						walk(append(paths, field.Name), field.Type)
+					}
+				}
+			}
+
+			walk(nil, reflect.TypeOf(atc.Plan{}))
+		})
 	})
 
 	Describe("Stdout", func() {

testflight/across_test.go

@@ -22,5 +22,14 @@ var _ = Describe("A pipeline containing an across step", func() {
 		Expect(watch).To(gbytes.Say("running across static2 dynamic2"))
 		Expect(watch).To(gbytes.Say("running across static2 dynamic3"))
 		Expect(watch).To(gbytes.Say("running across static2 4"))
+
+		Expect(watch).To(gbytes.Say("pushing version: v_dynamic1"))
+		Expect(watch).To(gbytes.Say("fetching version: v_dynamic1"))
+		Expect(watch).To(gbytes.Say("pushing version: v_dynamic2"))
+		Expect(watch).To(gbytes.Say("fetching version: v_dynamic2"))
+		Expect(watch).To(gbytes.Say("pushing version: v_dynamic3"))
+		Expect(watch).To(gbytes.Say("fetching version: v_dynamic3"))
+		Expect(watch).To(gbytes.Say("pushing version: v_4"))
+		Expect(watch).To(gbytes.Say("fetching version: v_4"))
 	})
 })

testflight/fixtures/across.yml

@@ -27,3 +27,8 @@ jobs:
       run:
         path: echo
         args: ["running across ((.:static)) ((.:dynamic))"]
+  - across:
+    - var: dynamic
+      values: ((.:dynamic))
+    put: mock
+    params: {version: "v_((.:dynamic))"}