From bc87208a33fb617c6f520ffa3af1dab4820bd01c Mon Sep 17 00:00:00 2001
From: Stephen Muth <smuth4@users.noreply.github.com>
Date: Mon, 2 Jan 2023 00:36:57 -0500
Subject: [PATCH] Rework status constraint logic for successes (#1515)

Since "success" and "failure" are the only two possible values, and
"success" is considered to be included by default, the existing code can
also be simplified a little.

This has the side effect of ignoring the "exclude" part of the
constraint completely. I put it in the tests just to make sure the
workaround in
https://github.com/woodpecker-ci/woodpecker/issues/1181#issuecomment-1347253585
continues to work as expected, but couldn't think of any legitimate use
cases for it.

Fixes #1181
---
 pipeline/frontend/yaml/compiler/convert.go    |  7 +++----
 .../frontend/yaml/constraint/constraint.go    | 19 ++++++++++++-------
 .../yaml/constraint/constraint_test.go        | 18 ++++++++++++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/pipeline/frontend/yaml/compiler/convert.go b/pipeline/frontend/yaml/compiler/convert.go
index 5ff01b99e..37d899e01 100644
--- a/pipeline/frontend/yaml/compiler/convert.go
+++ b/pipeline/frontend/yaml/compiler/convert.go
@@ -134,11 +134,10 @@ func (c *Compiler) createProcess(name string, container *yaml.Container, section
 		cpuSet = c.reslimit.CPUSet
 	}
 
-	// all constraints must exclude success.
-	onSuccess := container.When.IsEmpty() ||
-		!container.When.ExcludesStatus("success")
+	// at least one constraint contain status success, or all constraints have no status set
+	onSuccess := container.When.IncludesStatusSuccess()
 	// at least one constraint must include the status failure.
-	onFailure := container.When.IncludesStatus("failure")
+	onFailure := container.When.IncludesStatusFailure()
 
 	failure := container.Failure
 	if container.Failure == "" {
diff --git a/pipeline/frontend/yaml/constraint/constraint.go b/pipeline/frontend/yaml/constraint/constraint.go
index 2557e0e13..6db26f69b 100644
--- a/pipeline/frontend/yaml/constraint/constraint.go
+++ b/pipeline/frontend/yaml/constraint/constraint.go
@@ -79,9 +79,9 @@ func (when *When) Match(metadata frontend.Metadata, global bool) (bool, error) {
 	return false, nil
 }
 
-func (when *When) IncludesStatus(status string) bool {
+func (when *When) IncludesStatusFailure() bool {
 	for _, c := range when.Constraints {
-		if c.Status.Includes(status) {
+		if c.Status.Includes("failure") {
 			return true
 		}
 	}
@@ -89,14 +89,19 @@ func (when *When) IncludesStatus(status string) bool {
 	return false
 }
 
-func (when *When) ExcludesStatus(status string) bool {
+func (when *When) IncludesStatusSuccess() bool {
+	// "success" acts differently than "failure" in that it's
+	// presumed to be included unless it's specifically not part
+	// of the list
+	if when.IsEmpty() {
+		return true
+	}
 	for _, c := range when.Constraints {
-		if !c.Status.Excludes(status) {
-			return false
+		if len(c.Status.Include) == 0 || c.Status.Includes("success") {
+			return true
 		}
 	}
-
-	return len(when.Constraints) > 0
+	return false
 }
 
 // False if (any) non local
diff --git a/pipeline/frontend/yaml/constraint/constraint_test.go b/pipeline/frontend/yaml/constraint/constraint_test.go
index f5fec49be..3c72f59b0 100644
--- a/pipeline/frontend/yaml/constraint/constraint_test.go
+++ b/pipeline/frontend/yaml/constraint/constraint_test.go
@@ -381,6 +381,24 @@ func TestConstraintMap(t *testing.T) {
 	}
 }
 
+func TestConstraintStatusSuccess(t *testing.T) {
+	testdata := []struct {
+		conf string
+		want bool
+	}{
+		{conf: "", want: true},
+		{conf: "{status: [failure]}", want: false},
+		{conf: "{status: [success]}", want: true},
+		{conf: "{status: [failure, success]}", want: true},
+		{conf: "{status: {exclude: [success], include: [failure]}}", want: false},
+		{conf: "{status: {exclude: [failure], include: [success]}}", want: true},
+	}
+	for _, test := range testdata {
+		c := parseConstraints(t, test.conf)
+		assert.Equal(t, test.want, c.IncludesStatusSuccess(), "when: '%s'", test.conf)
+	}
+}
+
 func TestConstraints(t *testing.T) {
 	testdata := []struct {
 		desc string