Fix actions skipped commit status indicator

[badhezi]

Addresses #34500

[silverwind]

Seems good, but I wonder whether the other two missing statuses from action model should also be represented in commit status. Currently "blocked" and "unknown" are missing:

gitea/models/actions/status.go

Lines 15 to 24 in 14bb8f7

	const (
	StatusUnknown Status = iota // 0, consistent with runnerv1.Result_RESULT_UNSPECIFIED
	StatusSuccess // 1, consistent with runnerv1.Result_RESULT_SUCCESS
	StatusFailure // 2, consistent with runnerv1.Result_RESULT_FAILURE
	StatusCancelled // 3, consistent with runnerv1.Result_RESULT_CANCELLED
	StatusSkipped // 4, consistent with runnerv1.Result_RESULT_SKIPPED
	StatusWaiting // 5, isn't a runnerv1.Result
	StatusRunning // 6, isn't a runnerv1.Result
	StatusBlocked // 7, isn't a runnerv1.Result
	)

[silverwind]

Change is LGMT but we should review the other two potential missing states later.

[ChristopherHX]

For consistency maybe add add

// IsSkipped represents if commit status state is skipped
func (css CommitStatusState) IsSkipped() bool {
	return css == CommitStatusSkipped
}

[ChristopherHX]

Are commit statuses correctly merged with this change?

In Pull Request Status / Merged Commit Status Icon

I looked at other locations of the other constants.

e.g. assign skipped value 5

The compare method returns false in both directions if not part of commitStatusPriorities e.g. making a stable sort impossible.

[badhezi]

Thanks for pointing this out.
Im not sure if semantically skipped is "better than" success, anyhow ill assign 5 to it and we will see if anyone else has feedback.

[ChristopherHX]

I can read and read over the nobetterthan function, somehow I tend to get confused

This is a "no better than" table, negation.... Error has value 0, because it is a better status than warning, pending, success.
e.g. if we start with skipped/success, we end with error if a single status is error

After reading my suggestion here seems to be not required, but I found something else about PRs.
NoBetterThan is never used inverted and having no value defined for skipped would have a similar outcome if every other value is already in this table.

[ChristopherHX]

To make Pullrequests work correctly with required checks we might need to look at this proposed additional change:

diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go
index fee0da214c..e600d8caf9 100644
--- a/modules/structs/commit_status.go
+++ b/modules/structs/commit_status.go
@@ -57,7 +57,7 @@ func (css CommitStatusState) IsPending() bool {
 
 // IsSuccess represents if commit status state is success
 func (css CommitStatusState) IsSuccess() bool {
-	return css == CommitStatusSuccess
+	return css == CommitStatusSuccess || css == CommitStatusSkipped
 }
 
 // IsError represents if commit status state is error
diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go
index 0bfff21746..2e2570d43a 100644
--- a/services/pull/commit_status.go
+++ b/services/pull/commit_status.go
@@ -58,7 +58,7 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 		}
 	}
 
-	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
+	if matchedCount == 0 && returnedStatus.IsSuccess() {
 		status := git_model.CalcCommitStatus(commitStatuses)
 		if status != nil {
 			return status.State
@@ -74,7 +74,7 @@ func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requ
 	// 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 {
+		if status == nil || !status.State.IsSuccess() {
 			return false
 		}
 		return true
@@ -84,7 +84,7 @@ func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requ
 		var found bool
 		for _, commitStatus := range commitStatuses {
 			if commitStatus.Context == ctx {
-				if commitStatus.State != structs.CommitStatusSuccess {
+				if !commitStatus.State.IsSuccess() {
 					return false
 				}
 

• The new Commit Status is not handled in the PullRequest UI
  • My diff would change the IsSuccess function to return true for skipped as well to align with GitHub Actions (skipped <==> success)
• Code expecting Success status check within gitea is no longer working the same way

Maybe better wait for feedback of another maintainer instead of applying this, my knowledge confidence in this area of Gitea is not high...

[ChristopherHX]

I can read and read over the nobetterthan function, somehow I tend to get confused

This is a "no better than" table, negation.... Error has value 0, because it is a better status than warning, pending, success.
e.g. if we start with skipped/success, we end with error if a single status is error

After reading my suggestion here seems to be not required, but I found something else about PRs.
NoBetterThan is never used inverted and having no value defined for skipped would have a similar outcome if every other value is already in this table.

[wxiaoguang]

 // IsSuccess represents if commit status state is success
 func (css CommitStatusState) IsSuccess() bool {
-	return css == CommitStatusSuccess
+	return css == CommitStatusSuccess || css == CommitStatusSkipped
 }

I don't think we can make IsSuccess behave like this. It introduces more ambiguity. skipped is skipped, not success.

[wxiaoguang]

To make Pullrequests work correctly with required checks we might need to look at this proposed additional change:

-> I am not sure whether there is any problem in current implementation, but if we'd like to handle all statuses together, I recalled these two: Fix incomplete Actions status aggregations #32859 , Improve Actions status aggregations (#32860) , maybe related.

[lunny]

We need to diff between commit status and combined status. A combined status is a merge of multiple commit status for the final result which could be displayed in commit list or dashboard.

Commit Status could have skipped, "pending", "success", "error", "failure", "warning"
but combined status should have only failure, pending and success.

[ChristopherHX]

Commit Status could have skipped, "pending", "success", "error", "failure", "warning"
but combined status should have only failure, pending and success.

Yes this sounds good, the Pullrequest UI only consumes the combined status, if the function combine would then convert skipped to success this should work.

-> I am not sure whether there is any problem in current implementation

Current implementation of Gitea main is good, new problems only appear due to skipped mapped to a new commitstatusskipped. As far I understood the code.

I don't think we can make IsSuccess behave like this. It introduces more ambiguity. skipped is skipped, not success.

Add IsSuccessOrSkipped and use it or inline are my other ideas here.
AFAIK The suggestion from lunny would solve the UI part, the code part problem remains when doing that.

EDIT lunny your suggestion could make the commit summary render a green checkmark instead of skipped if everything is skipped.

Also replace IsSuccess with IsSuccessOrSkipped in the Pullrequest UI.

At least this seems to be not like just add a new commit status and everything works.