Merge pull request '[gitea] week 2024-52 cherry pick (gitea/main -> forgejo)' (#6342) from earl-warren/wcp/2024-52 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6342
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

models/actions/run_job.go

@@ -153,28 +153,34 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col
 }
 
 func AggregateJobStatus(jobs []*ActionRunJob) Status {
-	allDone := true
-	allWaiting := true
-	hasFailure := false
+	allSuccessOrSkipped := len(jobs) != 0
+	allSkipped := len(jobs) != 0
+	var hasFailure, hasCancelled, hasWaiting, hasRunning, hasBlocked bool
 	for _, job := range jobs {
-		if !job.Status.IsDone() {
-			allDone = false
-		}
-		if job.Status != StatusWaiting && !job.Status.IsDone() {
-			allWaiting = false
-		}
-		if job.Status == StatusFailure || job.Status == StatusCancelled {
-			hasFailure = true
-		}
+		allSuccessOrSkipped = allSuccessOrSkipped && (job.Status == StatusSuccess || job.Status == StatusSkipped)
+		allSkipped = allSkipped && job.Status == StatusSkipped
+		hasFailure = hasFailure || job.Status == StatusFailure
+		hasCancelled = hasCancelled || job.Status == StatusCancelled
+		hasWaiting = hasWaiting || job.Status == StatusWaiting
+		hasRunning = hasRunning || job.Status == StatusRunning
+		hasBlocked = hasBlocked || job.Status == StatusBlocked
 	}
-	if allDone {
-		if hasFailure {
-			return StatusFailure
-		}
+	switch {
+	case allSkipped:
+		return StatusSkipped
+	case allSuccessOrSkipped:
 		return StatusSuccess
-	}
-	if allWaiting {
+	case hasCancelled:
+		return StatusCancelled
+	case hasFailure:
+		return StatusFailure
+	case hasRunning:
+		return StatusRunning
+	case hasWaiting:
 		return StatusWaiting
+	case hasBlocked:
+		return StatusBlocked
+	default:
+		return StatusUnknown // it shouldn't happen
 	}
-	return StatusRunning
 }

models/actions/run_job_status_test.go

@@ -0,0 +1,85 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAggregateJobStatus(t *testing.T) {
+	testStatuses := func(expected Status, statuses []Status) {
+		t.Helper()
+		var jobs []*ActionRunJob
+		for _, v := range statuses {
+			jobs = append(jobs, &ActionRunJob{Status: v})
+		}
+		actual := AggregateJobStatus(jobs)
+		if !assert.Equal(t, expected, actual) {
+			var statusStrings []string
+			for _, s := range statuses {
+				statusStrings = append(statusStrings, s.String())
+			}
+			t.Errorf("AggregateJobStatus(%v) = %v, want %v", statusStrings, statusNames[actual], statusNames[expected])
+		}
+	}
+
+	cases := []struct {
+		statuses []Status
+		expected Status
+	}{
+		// unknown cases, maybe it shouldn't happen in real world
+		{[]Status{}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusSuccess}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusSkipped}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusFailure}, StatusFailure},
+		{[]Status{StatusUnknown, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusUnknown, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusUnknown, StatusRunning}, StatusRunning},
+		{[]Status{StatusUnknown, StatusBlocked}, StatusBlocked},
+
+		// success with other status
+		{[]Status{StatusSuccess}, StatusSuccess},
+		{[]Status{StatusSuccess, StatusSkipped}, StatusSuccess}, // skipped doesn't affect success
+		{[]Status{StatusSuccess, StatusFailure}, StatusFailure},
+		{[]Status{StatusSuccess, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusSuccess, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusSuccess, StatusRunning}, StatusRunning},
+		{[]Status{StatusSuccess, StatusBlocked}, StatusBlocked},
+
+		// any cancelled, then cancelled
+		{[]Status{StatusCancelled}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusSuccess}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusSkipped}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusFailure}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusWaiting}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusRunning}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusBlocked}, StatusCancelled},
+
+		// failure with other status, fail fast
+		// Should "running" win? Maybe no: old code does make "running" win, but GitHub does fail fast.
+		{[]Status{StatusFailure}, StatusFailure},
+		{[]Status{StatusFailure, StatusSuccess}, StatusFailure},
+		{[]Status{StatusFailure, StatusSkipped}, StatusFailure},
+		{[]Status{StatusFailure, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusFailure, StatusWaiting}, StatusFailure},
+		{[]Status{StatusFailure, StatusRunning}, StatusFailure},
+		{[]Status{StatusFailure, StatusBlocked}, StatusFailure},
+
+		// skipped with other status
+		// TODO: need to clarify whether a PR with "skipped" job status is considered as "mergeable" or not.
+		{[]Status{StatusSkipped}, StatusSkipped},
+		{[]Status{StatusSkipped, StatusSuccess}, StatusSuccess},
+		{[]Status{StatusSkipped, StatusFailure}, StatusFailure},
+		{[]Status{StatusSkipped, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusSkipped, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusSkipped, StatusRunning}, StatusRunning},
+		{[]Status{StatusSkipped, StatusBlocked}, StatusBlocked},
+	}
+
+	for _, c := range cases {
+		testStatuses(c.expected, c.statuses)
+	}
+}

models/fixtures/action_run.yml

@@ -432,6 +432,25 @@
   updated: 1683636626
   need_approval: 0
   approved_by: 0
+-
+  id: 794
+  title: "job output"
+  repo_id: 4
+  owner_id: 1
+  workflow_id: "test.yaml"
+  index: 190
+  trigger_user_id: 1
+  ref: "refs/heads/test"
+  commit_sha: "c2d72f548424103f01ee1dc02889c1e2bff816b0"
+  event: "push"
+  is_fork_pull_request: 0
+  status: 1
+  started: 1683636528
+  stopped: 1683636626
+  created: 1683636108
+  updated: 1683636626
+  need_approval: 0
+  approved_by: 0
 -
   id: 891
   title: "update actions"

models/fixtures/branch.yml

@@ -45,3 +45,15 @@
   is_deleted: false
   deleted_by_id: 0
   deleted_unix: 0
+
+-
+  id: 15
+  repo_id: 4
+  name: 'master'
+  commit_id: 'c7cd3cd144e6d23c9d6f3d07e52b2c1a956e0338'
+  commit_message: 'add Readme'
+  commit_time: 1588147171
+  pusher_id: 13
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0

routers/web/repo/actions/actions.go

@@ -5,6 +5,7 @@ package actions
 
 import (
 	"bytes"
+	stdCtx "context"
 	"fmt"
 	"net/http"
 	"slices"
@@ -224,7 +225,7 @@ func List(ctx *context.Context) {
 		return
 	}
 
-	if err := loadIsRefDeleted(ctx, runs); err != nil {
+	if err := loadIsRefDeleted(ctx, ctx.Repo.Repository.ID, runs); err != nil {
 		log.Error("LoadIsRefDeleted", err)
 	}
 
@@ -254,7 +255,7 @@ func List(ctx *context.Context) {
 
 // loadIsRefDeleted loads the IsRefDeleted field for each run in the list.
 // TODO: move this function to models/actions/run_list.go but now it will result in a circular import.
-func loadIsRefDeleted(ctx *context.Context, runs actions_model.RunList) error {
+func loadIsRefDeleted(ctx stdCtx.Context, repoID int64, runs actions_model.RunList) error {
 	branches := make(container.Set[string], len(runs))
 	for _, run := range runs {
 		refName := git.RefName(run.Ref)
@@ -266,14 +267,14 @@ func loadIsRefDeleted(ctx *context.Context, runs actions_model.RunList) error {
 		return nil
 	}
 
-	branchInfos, err := git_model.GetBranches(ctx, ctx.Repo.Repository.ID, branches.Values(), false)
+	branchInfos, err := git_model.GetBranches(ctx, repoID, branches.Values(), false)
 	if err != nil {
 		return err
 	}
 	branchSet := git_model.BranchesToNamesSet(branchInfos)
 	for _, run := range runs {
 		refName := git.RefName(run.Ref)
-		if refName.IsBranch() && !branchSet.Contains(run.Ref) {
+		if refName.IsBranch() && !branchSet.Contains(refName.ShortName()) {
 			run.IsRefDeleted = true
 		}
 	}

routers/web/repo/actions/actions_test.go

@@ -0,0 +1,33 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"testing"
+
+	actions_model "code.gitea.io/gitea/models/actions"
+	"code.gitea.io/gitea/models/db"
+	unittest "code.gitea.io/gitea/models/unittest"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_loadIsRefDeleted(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	runs, total, err := db.FindAndCount[actions_model.ActionRun](db.DefaultContext,
+		actions_model.FindRunOptions{RepoID: 4, Ref: "refs/heads/test"})
+	require.NoError(t, err)
+	assert.Len(t, runs, 1)
+	assert.EqualValues(t, 1, total)
+	for _, run := range runs {
+		assert.False(t, run.IsRefDeleted)
+	}
+
+	require.NoError(t, loadIsRefDeleted(db.DefaultContext, 4, runs))
+	for _, run := range runs {
+		assert.True(t, run.IsRefDeleted)
+	}
+}

routers/web/repo/actions/main_test.go

@@ -0,0 +1,14 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/unittest"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m)
+}

services/convert/pull.go

@@ -29,6 +29,11 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 		err        error
 	)
 
+	if err = pr.LoadIssue(ctx); err != nil {
+		log.Error("pr.LoadIssue[%d]: %v", pr.ID, err)
+		return nil
+	}
+
 	if err = pr.Issue.LoadRepo(ctx); err != nil {
 		log.Error("pr.Issue.LoadRepo[%d]: %v", pr.ID, err)
 		return nil

services/webhook/notifier.go

@@ -409,6 +409,10 @@ func (m *webhookNotifier) CreateIssueComment(ctx context.Context, doer *user_mod
 	var pullRequest *api.PullRequest
 	if issue.IsPull {
 		eventType = webhook_module.HookEventPullRequestComment
+		if err := issue.LoadPullRequest(ctx); err != nil {
+			log.Error("LoadPullRequest: %v", err)
+			return
+		}
 		pullRequest = convert.ToAPIPullRequest(ctx, issue.PullRequest, doer)
 	} else {
 		eventType = webhook_module.HookEventIssueComment

templates/package/content/container.tmpl

@@ -36,11 +36,13 @@
 				</thead>
 				<tbody>
 					{{range .PackageDescriptor.Metadata.Manifests}}
-					<tr>
+						{{if ne .Platform "unknown/unknown"}}
+						<tr>
 						<td><a href="{{$.PackageDescriptor.PackageWebLink}}/{{PathEscape .Digest}}">{{.Digest}}</a></td>
 						<td>{{.Platform}}</td>
 						<td>{{ctx.Locale.TrSize .Size}}</td>
-					</tr>
+						</tr>
+						{{end}}
 					{{end}}
 				</tbody>
 			</table>

templates/repo/actions/status.tmpl

@@ -17,13 +17,15 @@
 	{{svg "octicon-check-circle-fill" $size (printf "text green %s" $className)}}
 {{else if eq .status "skipped"}}
 	{{svg "octicon-skip" $size (printf "text grey %s" $className)}}
+{{else if eq .status "cancelled"}}
+	{{svg "octicon-stop" $size (printf "text grey %s" $className)}}
 {{else if eq .status "waiting"}}
 	{{svg "octicon-clock" $size (printf "text yellow %s" $className)}}
 {{else if eq .status "blocked"}}
 	{{svg "octicon-blocked" $size (printf "text yellow %s" $className)}}
 {{else if eq .status "running"}}
 	{{svg "octicon-meter" $size (printf "text yellow job-status-rotate %s" $className)}}
-{{else if or (eq .status "failure") or (eq .status "cancelled") or (eq .status "unknown")}}
+{{else}}{{/*failure, unknown*/}}
 	{{svg "octicon-x-circle-fill" $size (printf "text red %s" $className)}}
 {{end}}
 </span>

web_src/js/components/ActionRunStatus.vue

@@ -28,12 +28,13 @@ export default {
 };
 </script>
 <template>
-  <span class="tw-flex tw-items-center" :data-tooltip-content="localeStatus" v-if="status">
+  <span class="tw-flex tw-items-center" :data-tooltip-content="localeStatus ?? status" v-if="status">
     <SvgIcon name="octicon-check-circle-fill" class="text green" :size="size" :class-name="className" v-if="status === 'success'"/>
     <SvgIcon name="octicon-skip" class="text grey" :size="size" :class-name="className" v-else-if="status === 'skipped'"/>
+    <SvgIcon name="octicon-stop" class="text yellow" :size="size" :class-name="className" v-else-if="status === 'cancelled'"/>
     <SvgIcon name="octicon-clock" class="text yellow" :size="size" :class-name="className" v-else-if="status === 'waiting'"/>
     <SvgIcon name="octicon-blocked" class="text yellow" :size="size" :class-name="className" v-else-if="status === 'blocked'"/>
     <SvgIcon name="octicon-meter" class="text yellow" :size="size" :class-name="'job-status-rotate ' + className" v-else-if="status === 'running'"/>
-    <SvgIcon name="octicon-x-circle-fill" class="text red" :size="size" v-else-if="['failure', 'cancelled', 'unknown'].includes(status)"/>
+    <SvgIcon name="octicon-x-circle-fill" class="text red" :size="size" v-else/><!-- failure, unknown -->
   </span>
 </template>

web_src/js/components/RepoActionView.vue

@@ -570,11 +570,13 @@ export function initRepositoryActionView() {
 
 .action-info-summary-title {
   display: flex;
+  align-items: center;
+  gap: 0.5em;
 }
 
 .action-info-summary-title-text {
   font-size: 20px;
-  margin: 0 0 0 8px;
+  margin: 0;
   flex: 1;
   overflow-wrap: anywhere;
 }

web_src/js/svg.js

@@ -64,6 +64,7 @@ import octiconSidebarCollapse from '../../public/assets/img/svg/octicon-sidebar-
 import octiconSidebarExpand from '../../public/assets/img/svg/octicon-sidebar-expand.svg';
 import octiconSkip from '../../public/assets/img/svg/octicon-skip.svg';
 import octiconStar from '../../public/assets/img/svg/octicon-star.svg';
+import octiconStop from '../../public/assets/img/svg/octicon-stop.svg';
 import octiconStrikethrough from '../../public/assets/img/svg/octicon-strikethrough.svg';
 import octiconSync from '../../public/assets/img/svg/octicon-sync.svg';
 import octiconTable from '../../public/assets/img/svg/octicon-table.svg';
@@ -138,6 +139,7 @@ const svgs = {
   'octicon-sidebar-expand': octiconSidebarExpand,
   'octicon-skip': octiconSkip,
   'octicon-star': octiconStar,
+  'octicon-stop': octiconStop,
   'octicon-strikethrough': octiconStrikethrough,
   'octicon-sync': octiconSync,
   'octicon-table': octiconTable,