Fix actions skipped commit status indicator (#34507)

Addresses #34500

Co-authored-by: wxiaoguang <[email protected]>

Author: badhezi

models/git/commit_status.go

@@ -230,18 +230,24 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) {
 
 // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc
 func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus {
+	// This function is widely used, but it is not quite right.
+	// Ideally it should return something like "CommitStatusSummary" with properly aggregated state.
+	// GitHub's behavior: if all statuses are "skipped", GitHub will return "success" as the combined status.
 	var lastStatus *CommitStatus
 	state := api.CommitStatusSuccess
 	for _, status := range statuses {
-		if status.State.NoBetterThan(state) {
+		if state == status.State || status.State.HasHigherPriorityThan(state) {
 			state = status.State
 			lastStatus = status
 		}
 	}
 	if lastStatus == nil {
 		if len(statuses) > 0 {
+			// FIXME: a bad case: Gitea just returns the first commit status, its status is "skipped" in this case.
 			lastStatus = statuses[0]
 		} else {
+			// FIXME: another bad case: if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty.
+			// Frontend code (tmpl&vue) sometimes depend on the empty fields to skip rendering commit status elements (need to double check in the future)
 			lastStatus = &CommitStatus{}
 		}
 	}

models/git/commit_status_summary.go

@@ -59,7 +59,11 @@ func UpdateCommitStatusSummary(ctx context.Context, repoID int64, sha string) er
 	if err != nil {
 		return err
 	}
-	state := CalcCommitStatus(commitStatuses)
+	// it guarantees that commitStatuses is not empty because this function is always called after a commit status is created
+	if len(commitStatuses) == 0 {
+		setting.PanicInDevOrTesting("no commit statuses found for repo %d and sha %s", repoID, sha)
+	}
+	state := CalcCommitStatus(commitStatuses) // non-empty commitStatuses is guaranteed
 	// mysql will return 0 when update a record which state hasn't been changed which behaviour is different from other database,
 	// so we need to use insert in on duplicate
 	if setting.Database.Type.IsMySQL() {

modules/structs/commit_status.go

@@ -18,6 +18,8 @@ const (
 	CommitStatusFailure CommitStatusState = "failure"
 	// CommitStatusWarning is for when the CommitStatus is Warning
 	CommitStatusWarning CommitStatusState = "warning"
+	// CommitStatusSkipped is for when CommitStatus is Skipped
+	CommitStatusSkipped CommitStatusState = "skipped"
 )
 
 var commitStatusPriorities = map[CommitStatusState]int{
@@ -26,25 +28,17 @@ var commitStatusPriorities = map[CommitStatusState]int{
 	CommitStatusWarning: 2,
 	CommitStatusPending: 3,
 	CommitStatusSuccess: 4,
+	CommitStatusSkipped: 5,
 }
 
 func (css CommitStatusState) String() string {
 	return string(css)
 }
 
-// NoBetterThan returns true if this State is no better than the given State
-// This function only handles the states defined in CommitStatusPriorities
-func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool {
-	// NoBetterThan only handles the 5 states above
-	if _, exist := commitStatusPriorities[css]; !exist {
-		return false
-	}
-
-	if _, exist := commitStatusPriorities[css2]; !exist {
-		return false
-	}
-
-	return commitStatusPriorities[css] <= commitStatusPriorities[css2]
+// HasHigherPriorityThan returns true if this state has higher priority than the other
+// Undefined states are considered to have the highest priority like CommitStatusError(0)
+func (css CommitStatusState) HasHigherPriorityThan(other CommitStatusState) bool {
+	return commitStatusPriorities[css] < commitStatusPriorities[other]
 }
 
 // IsPending represents if commit status state is pending

modules/structs/commit_status_test.go

@@ -10,165 +10,21 @@ import (
 )
 
 func TestNoBetterThan(t *testing.T) {
-	type args struct {
-		css  CommitStatusState
-		css2 CommitStatusState
-	}
-	var unExpectedState CommitStatusState
 	tests := []struct {
-		name string
-		args args
-		want bool
+		s1, s2 CommitStatusState
+		higher bool
 	}{
-		{
-			name: "success is no better than success",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "success is no better than pending",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusPending,
-			},
-			want: false,
-		},
-		{
-			name: "success is no better than failure",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusFailure,
-			},
-			want: false,
-		},
-		{
-			name: "success is no better than error",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "pending is no better than success",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "pending is no better than pending",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "pending is no better than failure",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusFailure,
-			},
-			want: false,
-		},
-		{
-			name: "pending is no better than error",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "failure is no better than success",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than pending",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than failure",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusFailure,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than error",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "error is no better than success",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than pending",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than failure",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusFailure,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than error",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusError,
-			},
-			want: true,
-		},
-		{
-			name: "unExpectedState is no better than success",
-			args: args{
-				css:  unExpectedState,
-				css2: CommitStatusSuccess,
-			},
-			want: false,
-		},
-		{
-			name: "unExpectedState is no better than unExpectedState",
-			args: args{
-				css:  unExpectedState,
-				css2: unExpectedState,
-			},
-			want: false,
-		},
+		{CommitStatusError, CommitStatusFailure, true},
+		{CommitStatusFailure, CommitStatusWarning, true},
+		{CommitStatusWarning, CommitStatusPending, true},
+		{CommitStatusPending, CommitStatusSuccess, true},
+		{CommitStatusSuccess, CommitStatusSkipped, true},
+
+		{CommitStatusError, "unknown-xxx", false},
+		{"unknown-xxx", CommitStatusFailure, true},
 	}
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := tt.args.css.NoBetterThan(tt.args.css2)
-			assert.Equal(t, tt.want, result)
-		})
+		assert.Equal(t, tt.higher, tt.s1.HasHigherPriorityThan(tt.s2), "s1=%s, s2=%s, expected=%v", tt.s1, tt.s2, tt.higher)
 	}
+	assert.False(t, CommitStatusError.HasHigherPriorityThan(CommitStatusError))
 }

services/actions/commit_status.go

@@ -149,12 +149,14 @@ func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er
 
 func toCommitStatus(status actions_model.Status) api.CommitStatusState {
 	switch status {
-	case actions_model.StatusSuccess, actions_model.StatusSkipped:
+	case actions_model.StatusSuccess:
 		return api.CommitStatusSuccess
 	case actions_model.StatusFailure, actions_model.StatusCancelled:
 		return api.CommitStatusFailure
 	case actions_model.StatusWaiting, actions_model.StatusBlocked, actions_model.StatusRunning:
 		return api.CommitStatusPending
+	case actions_model.StatusSkipped:
+		return api.CommitStatusSkipped
 	default:
 		return api.CommitStatusError
 	}

services/convert/status.go

@@ -42,13 +42,14 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 		SHA:        statuses[0].SHA,
 		TotalCount: len(statuses),
 		Repository: repo,
-		URL:        "",
+		URL:        "", // never set or used?
+		State:      api.CommitStatusSuccess,
 	}
 
 	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
 	for _, status := range statuses {
 		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
-		if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) {
+		if status.State.HasHigherPriorityThan(retStatus.State) {
 			retStatus.State = status.State
 		}
 	}
@@ -57,9 +58,13 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 	// > failure if any of the contexts report as error or failure
 	// > pending if there are no statuses or a context is pending
 	// > success if the latest status for all contexts is success
-	if retStatus.State.IsError() {
-		retStatus.State = api.CommitStatusFailure
+	switch retStatus.State {
+	case api.CommitStatusSkipped:
+		retStatus.State = api.CommitStatusSuccess // all skipped means success
+	case api.CommitStatusPending, api.CommitStatusSuccess:
+		// use the current state for pending or success
+	default:
+		retStatus.State = api.CommitStatusFailure // otherwise, it is a failure
 	}
-
 	return retStatus
 }

services/pull/commit_status.go

@@ -46,57 +46,31 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 
 			// If required rule not match any action, then it is pending
 			if targetStatus == "" {
-				if structs.CommitStatusPending.NoBetterThan(returnedStatus) {
+				if structs.CommitStatusPending.HasHigherPriorityThan(returnedStatus) {
 					returnedStatus = structs.CommitStatusPending
 				}
 				break
 			}
 
-			if targetStatus.NoBetterThan(returnedStatus) {
+			if targetStatus.HasHigherPriorityThan(returnedStatus) {
 				returnedStatus = targetStatus
 			}
 		}
 	}
 
 	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
-		status := git_model.CalcCommitStatus(commitStatuses)
-		if status != nil {
-			return status.State
+		if len(commitStatuses) == 0 {
+			// "no statuses" should mean "pending"
+			return structs.CommitStatusPending
 		}
-		return structs.CommitStatusSuccess
-	}
-
-	return returnedStatus
-}
-
-// IsCommitStatusContextSuccess returns true if all required status check contexts succeed.
-func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requiredContexts []string) bool {
-	// If no specific context is required, require that last commit status is a success
-	if len(requiredContexts) == 0 {
 		status := git_model.CalcCommitStatus(commitStatuses)
-		if status == nil || status.State != structs.CommitStatusSuccess {
-			return false
+		if status.State == structs.CommitStatusSkipped {
+			return structs.CommitStatusSuccess // if all statuses are skipped, return success
 		}
-		return true
+		return status.State
 	}
 
-	for _, ctx := range requiredContexts {
-		var found bool
-		for _, commitStatus := range commitStatuses {
-			if commitStatus.Context == ctx {
-				if commitStatus.State != structs.CommitStatusSuccess {
-					return false
-				}
-
-				found = true
-				break
-			}
-		}
-		if !found {
-			return false
-		}
-	}
-	return true
+	return returnedStatus
 }
 
 // IsPullCommitStatusPass returns if all required status checks PASS