convert onFailure breakpoint to a bool

based on tektoncd/community#572 (review) --- the onFailure breakpoint is updated to be a bool to avoid considering "onFailure" as reserved string.
waveywaves · Jan 20, 2022 · 044e9d6 · 044e9d6
1 parent e752c2f
commit 044e9d6
Show file tree

Hide file tree

Showing 11 changed files with 88 additions and 78 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go
diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json
@@ -2075,16 +2075,22 @@
         }
       }
     },
+    "v1beta1.TaskRunBreakpoint": {
+      "description": "TaskRunBreakpoint defines the breakpoint config for a particular TaskRun",
+      "type": "object",
+      "properties": {
+        "onFailure": {
+          "type": "boolean",
+          "default": false
+        }
+      }
+    },
     "v1beta1.TaskRunDebug": {
-      "description": "TaskRunDebug defines the breakpoint config for a particular TaskRun",
+      "description": "TaskRunDebug defines the debug config for a particular TaskRun",
       "type": "object",
       "properties": {
         "breakpoint": {
-          "type": "array",
-          "items": {
-            "type": "string",
-            "default": ""
-          }
+          "$ref": "#/definitions/v1beta1.TaskRunBreakpoint"
         }
       }
     },

diff --git a/pkg/apis/pipeline/v1beta1/taskrun_types.go b/pkg/apis/pipeline/v1beta1/taskrun_types.go
@@ -72,10 +72,16 @@ const (
 	TaskRunSpecStatusCancelled = "TaskRunCancelled"
 )
 
-// TaskRunDebug defines the breakpoint config for a particular TaskRun
+// TaskRunDebug defines the debug config for a particular TaskRun
 type TaskRunDebug struct {
 	// +optional
-	Breakpoint []string `json:"breakpoint,omitempty"`
+	Breakpoint *TaskRunBreakpoint `json:"breakpoint,omitempty"`
+}
+
+// TaskRunBreakpoint defines the breakpoint config for a particular TaskRun
+type TaskRunBreakpoint struct {
+	// +optional
+	OnFailure bool `json:"onFailure"`
 }
 
 // TaskRunInputs holds the input values that this task was invoked with.

diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go
@@ -104,17 +104,12 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	return errs
 }
 
-// validateDebug
+// validateDebug checks if the given debug options provided for the TaskRun make sense.
 func validateDebug(db *TaskRunDebug) (errs *apis.FieldError) {
-	breakpointOnFailure := "onFailure"
-	validBreakpoints := sets.NewString()
-	validBreakpoints.Insert(breakpointOnFailure)
+	// TODO:  Add breakpoints validation for beforeStep and afterStep
+	//validBreakpoints := sets.NewString()
+	//validBreakpoints.Insert()
 
-	for _, b := range db.Breakpoint {
-		if !validBreakpoints.Has(b) {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s is not a valid breakpoint. Available valid breakpoints include %s", b, validBreakpoints.List()), "breakpoint"))
-		}
-	}
 	return errs
 }
 

diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go
@@ -238,29 +238,6 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 		},
 		wantErr: apis.ErrInvalidValue("invalid bundle reference", "taskRef.bundle", "could not parse reference: invalid reference"),
 		wc:      enableTektonOCIBundles(t),
-	}, {
-		name: "using debug when apifields stable",
-		spec: v1beta1.TaskRunSpec{
-			TaskRef: &v1beta1.TaskRef{
-				Name: "my-task",
-			},
-			Debug: &v1beta1.TaskRunDebug{
-				Breakpoint: []string{"bReaKdAnCe"},
-			},
-		},
-		wantErr: apis.ErrDisallowedFields("debug"),
-	}, {
-		name: "invalid breakpoint",
-		spec: v1beta1.TaskRunSpec{
-			TaskRef: &v1beta1.TaskRef{
-				Name: "my-task",
-			},
-			Debug: &v1beta1.TaskRunDebug{
-				Breakpoint: []string{"breakito"},
-			},
-		},
-		wantErr: apis.ErrInvalidValue("breakito is not a valid breakpoint. Available valid breakpoints include [onFailure]", "debug.breakpoint"),
-		wc:      enableAlphaAPIFields,
 	}}
 	for _, ts := range tests {
 		t.Run(ts.name, func(t *testing.T) {

diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go
diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go
@@ -52,8 +52,6 @@ const (
 
 	stepPrefix    = "step-"
 	sidecarPrefix = "sidecar-"
-
-	breakpointOnFailure = "onFailure"
 )
 
 var (
@@ -144,14 +142,8 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe
 			argsForEntrypoint = append(argsForEntrypoint, resultArgument(steps, taskSpec.Results)...)
 		}
 
-		if breakpointConfig != nil && len(breakpointConfig.Breakpoint) > 0 {
-			breakpoints := breakpointConfig.Breakpoint
-			for _, b := range breakpoints {
-				// TODO(TEP #0042): Add other breakpoints
-				if b == breakpointOnFailure {
-					argsForEntrypoint = append(argsForEntrypoint, "-breakpoint_on_failure")
-				}
-			}
+		if breakpointConfig != nil && breakpointConfig.Breakpoint.OnFailure {
+			argsForEntrypoint = append(argsForEntrypoint, "-breakpoint_on_failure")
 		}
 
 		cmd, args := s.Command, s.Args

diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go
@@ -123,7 +123,7 @@ func TestOrderContainersWithDebugOnFailure(t *testing.T) {
 		TerminationMessagePath: "/tekton/termination",
 	}}
 	taskRunDebugConfig := &v1beta1.TaskRunDebug{
-		Breakpoint: []string{"onFailure"},
+		Breakpoint: &v1beta1.TaskRunBreakpoint{OnFailure: true},
 	}
 	got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig)
 	if err != nil {

diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go
@@ -157,7 +157,7 @@ func TestPodBuild(t *testing.T) {
 		desc: "simple with breakpoint onFailure enabled, alpha api fields disabled",
 		trs: v1beta1.TaskRunSpec{
 			Debug: &v1beta1.TaskRunDebug{
-				Breakpoint: []string{breakpointOnFailure},
+				Breakpoint: &v1beta1.TaskRunBreakpoint{OnFailure: true},
 			},
 		},
 		ts: v1beta1.TaskSpec{
@@ -1660,7 +1660,7 @@ func TestPodBuildwithAlphaAPIEnabled(t *testing.T) {
 		desc: "simple with debug breakpoint onFailure",
 		trs: v1beta1.TaskRunSpec{
 			Debug: &v1beta1.TaskRunDebug{
-				Breakpoint: []string{breakpointOnFailure},
+				Breakpoint: &v1beta1.TaskRunBreakpoint{OnFailure: true},
 			},
 		},
 		ts: v1beta1.TaskSpec{

diff --git a/pkg/pod/script.go b/pkg/pod/script.go
@@ -95,7 +95,8 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta
 		VolumeMounts: []corev1.VolumeMount{writeScriptsVolumeMount, binMount},
 	}
 
-	breakpoints := []string{}
+	breakpointOnFailure := false
+	// TODO: init before and after breakpoints here
 	sideCarSteps := []v1beta1.Step{}
 	for _, step := range sidecars {
 		sidecarStep := v1beta1.Step{
@@ -107,15 +108,16 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta
 	}
 
 	// Add mounts for debug
-	if debugConfig != nil && len(debugConfig.Breakpoint) > 0 {
-		breakpoints = debugConfig.Breakpoint
+	if debugConfig != nil && debugConfig.Breakpoint != nil {
+		breakpointOnFailure = debugConfig.Breakpoint.OnFailure
+		// TODO: set before and after breakpoints here
 		placeScriptsInit.VolumeMounts = append(placeScriptsInit.VolumeMounts, debugScriptsVolumeMount)
 	}
 
-	convertedStepContainers := convertListOfSteps(steps, &placeScriptsInit, &placeScripts, breakpoints, "script")
+	convertedStepContainers := convertListOfSteps(steps, &placeScriptsInit, &placeScripts, breakpointOnFailure, "script")
 
-	// Pass empty breakpoint list in "sidecar step to container" converter to not rewrite the scripts and add breakpoints to sidecar
-	sidecarContainers := convertListOfSteps(sideCarSteps, &placeScriptsInit, &placeScripts, []string{}, "sidecar-script")
+	// Pass false value for breakpointOnFailure in "sidecar step to container" converter to not rewrite the scripts and add breakpoints to sidecar
+	sidecarContainers := convertListOfSteps(sideCarSteps, &placeScriptsInit, &placeScripts, false, "sidecar-script")
 	if placeScripts {
 		return &placeScriptsInit, convertedStepContainers, sidecarContainers
 	}
@@ -126,7 +128,7 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta
 //
 // It iterates through the list of steps (or sidecars), generates the script file name and heredoc termination string,
 // adds an entry to the init container args, sets up the step container to run the script, and sets the volume mounts.
-func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, placeScripts *bool, breakpoints []string, namePrefix string) []corev1.Container {
+func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, placeScripts *bool, breakpointOnFailure bool, namePrefix string) []corev1.Container {
 	containers := []corev1.Container{}
 	for i, s := range steps {
 		if s.Script == "" {
@@ -186,8 +188,9 @@ cat > ${scriptfile} << '%s'
 		}
 		steps[i].VolumeMounts = append(steps[i].VolumeMounts, scriptsVolumeMount)
 
-		// Add debug mounts if breakpoints are present
-		if len(breakpoints) > 0 {
+		// Add debug mounts if breakpointOnFailure is present
+		// TODO: change this to factor in changes to
+		if breakpointOnFailure {
 			debugInfoVolumeMount := corev1.VolumeMount{
 				Name:      debugInfoVolumeName,
 				MountPath: filepath.Join(debugInfoDir, fmt.Sprintf("%d", i)),
@@ -197,8 +200,8 @@ cat > ${scriptfile} << '%s'
 		containers = append(containers, steps[i].Container)
 	}
 
-	// Place debug scripts if breakpoints are enabled
-	if len(breakpoints) > 0 {
+	// Place debug scripts if breakpointOnFailure are enabled
+	if breakpointOnFailure {
 		type script struct {
 			name    string
 			content string

diff --git a/pkg/pod/script_test.go b/pkg/pod/script_test.go
@@ -247,7 +247,7 @@ script-3`,
 			Args:         []string{"my", "args"},
 		},
 	}}, []v1beta1.Sidecar{}, &v1beta1.TaskRunDebug{
-		Breakpoint: []string{breakpointOnFailure},
+		Breakpoint: &v1beta1.TaskRunBreakpoint{OnFailure: true},
 	})
 	wantInit := &corev1.Container{
 		Name:    "place-scripts",