mirror of
https://github.com/go-gitea/gitea.git
synced 2026-06-10 03:43:16 +02:00
fix(actions): keep distinct commit statuses for workflows sharing a name (#37834)
## Summary Two Gitea Actions workflow files that share the same `name:` and same job name produced identical commit-status `Context` strings. Because `GetLatestCommitStatus` groups by `context_hash` (derived from `Context`), only one row was shown on the PR page — see #35699. GitHub displays both rows even though they look identical. This change does the same: the displayed `Context` is unchanged, but `ContextHash` now mixes in the workflow file path so the two statuses remain distinct in the dedupe query. ## Notes - Workflows that omit `name:` now use the workflow file name in the `Context` (e.g. `ci.yaml / build (push)`) instead of an empty `/ build (push)`. This changes the `Context` string for unnamed workflows, so any required-status-check rule that referenced the old string must be updated after upgrade. - For statuses created before this change (hashed from `Context` alone), `createCommitStatus` reuses that legacy hash when a matching row is still present, so in-flight pending statuses are superseded rather than orphaned on upgrade. Fixes #35699 --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: silverwind <me@silverwind.io>
This commit is contained in:
parent
5fe77ad309
commit
63df886ba8
@ -505,13 +505,19 @@ func NewCommitStatus(ctx context.Context, opts NewCommitStatusOptions) error {
|
||||
opts.CommitStatus.Description = strings.TrimSpace(opts.CommitStatus.Description)
|
||||
opts.CommitStatus.Context = strings.TrimSpace(opts.CommitStatus.Context)
|
||||
opts.CommitStatus.TargetURL = strings.TrimSpace(opts.CommitStatus.TargetURL)
|
||||
opts.CommitStatus.ContextHash = strings.TrimSpace(opts.CommitStatus.ContextHash)
|
||||
opts.CommitStatus.SHA = opts.SHA.String()
|
||||
opts.CommitStatus.CreatorID = opts.Creator.ID
|
||||
opts.CommitStatus.RepoID = opts.Repo.ID
|
||||
opts.CommitStatus.Index = idx
|
||||
log.Debug("NewCommitStatus[%s, %s]: %d", opts.Repo.FullName(), opts.SHA, opts.CommitStatus.Index)
|
||||
|
||||
opts.CommitStatus.ContextHash = hashCommitStatusContext(opts.CommitStatus.Context)
|
||||
// Callers may pre-compute a ContextHash to keep entries that share a
|
||||
// human-readable Context separated (e.g. two workflow files with the
|
||||
// same `name:` — issue #35699). Only derive from Context when unset.
|
||||
if opts.CommitStatus.ContextHash == "" {
|
||||
opts.CommitStatus.ContextHash = HashCommitStatusContext(opts.CommitStatus.Context)
|
||||
}
|
||||
|
||||
// Insert new CommitStatus
|
||||
if err = db.Insert(ctx, opts.CommitStatus); err != nil {
|
||||
@ -529,8 +535,11 @@ type SignCommitWithStatuses struct {
|
||||
*asymkey_model.SignCommit
|
||||
}
|
||||
|
||||
// hashCommitStatusContext hash context
|
||||
func hashCommitStatusContext(context string) string {
|
||||
// HashCommitStatusContext returns the sha1 hash used to dedupe commit statuses
|
||||
// by Context. Callers that need to keep statuses with the same display Context
|
||||
// separated (e.g. distinct workflow files sharing a `name:`) can mix extra
|
||||
// disambiguating data into the input.
|
||||
func HashCommitStatusContext(context string) string {
|
||||
return fmt.Sprintf("%x", sha1.Sum([]byte(context)))
|
||||
}
|
||||
|
||||
|
||||
@ -139,10 +139,24 @@ func getCommitStatusEventNameAndCommitID(run *actions_model.ActionRun) (event, c
|
||||
func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, commitID string, run *actions_model.ActionRun, job *actions_model.ActionRunJob) error {
|
||||
// TODO: store workflow name as a field in ActionRun to avoid parsing
|
||||
runName := path.Base(run.WorkflowID)
|
||||
// fall back to the file name when the workflow has no non-blank `name:`
|
||||
if wfs, err := jobparser.Parse(job.WorkflowPayload); err == nil && len(wfs) > 0 {
|
||||
runName = wfs[0].Name
|
||||
if name := strings.TrimSpace(wfs[0].Name); name != "" {
|
||||
runName = name
|
||||
}
|
||||
}
|
||||
ctxName := strings.TrimSpace(fmt.Sprintf("%s / %s (%s)", runName, job.Name, event)) // git_model.NewCommitStatus also trims spaces
|
||||
// Mix the workflow file path into the hash so two workflow files that
|
||||
// share the same `name:` and job name produce distinct commit statuses
|
||||
// even though they render identically — matching GitHub's behavior
|
||||
// (issue #35699).
|
||||
ctxHash := git_model.HashCommitStatusContext(ctxName + "\x00" + run.WorkflowID)
|
||||
// Pre-fix rows were hashed from Context alone. If a pre-existing row with
|
||||
// the legacy hash is still the "latest" for this SHA, reuse that hash so
|
||||
// the new row supersedes it; otherwise the old pending status would stay
|
||||
// stuck forever (it lives in its own dedupe group). Only relevant for
|
||||
// in-flight workflows at upgrade time.
|
||||
legacyHash := git_model.HashCommitStatusContext(ctxName)
|
||||
state := toCommitStatus(job.Status)
|
||||
targetURL := fmt.Sprintf("%s/jobs/%d", run.Link(), job.ID)
|
||||
description := toCommitStatusDescription(job)
|
||||
@ -152,7 +166,13 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event,
|
||||
return fmt.Errorf("GetLatestCommitStatus: %w", err)
|
||||
}
|
||||
for _, v := range statuses {
|
||||
if v.Context == ctxName {
|
||||
if v.ContextHash == legacyHash && v.Context == ctxName {
|
||||
ctxHash = legacyHash
|
||||
break
|
||||
}
|
||||
}
|
||||
for _, v := range statuses {
|
||||
if v.ContextHash == ctxHash {
|
||||
if v.State == state && v.TargetURL == targetURL && v.Description == description {
|
||||
return nil
|
||||
}
|
||||
@ -166,6 +186,7 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event,
|
||||
TargetURL: targetURL,
|
||||
Description: description,
|
||||
Context: ctxName,
|
||||
ContextHash: ctxHash,
|
||||
State: state,
|
||||
CreatorID: creator.ID,
|
||||
}
|
||||
|
||||
@ -11,8 +11,10 @@ import (
|
||||
git_model "gitea.dev/models/git"
|
||||
repo_model "gitea.dev/models/repo"
|
||||
"gitea.dev/models/unittest"
|
||||
user_model "gitea.dev/models/user"
|
||||
actions_module "gitea.dev/modules/actions"
|
||||
"gitea.dev/modules/commitstatus"
|
||||
"gitea.dev/modules/git"
|
||||
"gitea.dev/modules/gitrepo"
|
||||
"gitea.dev/modules/timeutil"
|
||||
|
||||
@ -146,6 +148,158 @@ func TestGetCommitActionsStatusMap(t *testing.T) {
|
||||
assert.Empty(t, nilInfo.IconStatus(statuses[0]))
|
||||
}
|
||||
|
||||
// TestCreateCommitStatus_DistinctWorkflowFilesSameName covers issue #35699:
|
||||
// two workflow files with the same `name:` and same job name must produce
|
||||
// two distinct commit statuses, not be deduplicated into one.
|
||||
func TestCreateCommitStatus_DistinctWorkflowFilesSameName(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
|
||||
branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
|
||||
|
||||
payload := []byte(`
|
||||
name: test-run
|
||||
on: pull_request
|
||||
jobs:
|
||||
my-test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hi
|
||||
`)
|
||||
|
||||
for _, spec := range []struct {
|
||||
workflowID string
|
||||
runID, jobID int64
|
||||
}{
|
||||
{"workflow1.yaml", 99101, 99201},
|
||||
{"workflow2.yaml", 99102, 99202},
|
||||
} {
|
||||
run := &actions_model.ActionRun{
|
||||
ID: spec.runID, Index: spec.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
|
||||
WorkflowID: spec.workflowID, CommitSHA: branch.CommitID,
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), run))
|
||||
job := &actions_model.ActionRunJob{
|
||||
ID: spec.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
|
||||
Name: "my-test", Status: actions_model.StatusWaiting,
|
||||
WorkflowPayload: payload,
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), job))
|
||||
require.NoError(t, createCommitStatus(t.Context(), repo, "pull_request", branch.CommitID, run, job))
|
||||
}
|
||||
|
||||
statuses, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Both workflow files should produce a row even though the display
|
||||
// Context is identical — matching GitHub's behavior.
|
||||
hashes := map[string]struct{}{}
|
||||
targets := map[string]struct{}{}
|
||||
for _, st := range statuses {
|
||||
hashes[st.ContextHash] = struct{}{}
|
||||
targets[st.TargetURL] = struct{}{}
|
||||
assert.Equal(t, "test-run / my-test (pull_request)", st.Context)
|
||||
}
|
||||
assert.Len(t, hashes, 2, "expected distinct ContextHash per workflow file")
|
||||
assert.Len(t, targets, 2, "expected distinct TargetURL per workflow file")
|
||||
}
|
||||
|
||||
// TestCreateCommitStatus_LegacyHashRecovery covers the upgrade path: a pending
|
||||
// status created before the fix (hashed from Context alone) must still be
|
||||
// superseded by a follow-up event, instead of being orphaned in its own dedupe
|
||||
// group while a new row accumulates under the new hash.
|
||||
func TestCreateCommitStatus_LegacyHashRecovery(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
|
||||
branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
|
||||
|
||||
ctxName := "legacy.yaml / my-job (push)"
|
||||
legacyHash := git_model.HashCommitStatusContext(ctxName)
|
||||
sha, err := git.NewIDFromString(branch.CommitID)
|
||||
require.NoError(t, err)
|
||||
creator := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
|
||||
require.NoError(t, git_model.NewCommitStatus(t.Context(), git_model.NewCommitStatusOptions{
|
||||
Repo: repo,
|
||||
Creator: creator,
|
||||
SHA: sha,
|
||||
CommitStatus: &git_model.CommitStatus{
|
||||
State: commitstatus.CommitStatusPending,
|
||||
Context: ctxName,
|
||||
ContextHash: legacyHash,
|
||||
TargetURL: "https://example.invalid/legacy",
|
||||
Description: "Waiting to run",
|
||||
},
|
||||
}))
|
||||
|
||||
run := &actions_model.ActionRun{
|
||||
ID: 99301, Index: 99301, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
|
||||
WorkflowID: "legacy.yaml", CommitSHA: branch.CommitID,
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), run))
|
||||
job := &actions_model.ActionRunJob{
|
||||
ID: 99302, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
|
||||
Name: "my-job", Status: actions_model.StatusSuccess,
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), job))
|
||||
require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job))
|
||||
|
||||
latest, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll)
|
||||
require.NoError(t, err)
|
||||
// The new row must reuse the legacy hash so GetLatestCommitStatus returns
|
||||
// only one entry for this Context — the success, not the orphaned pending.
|
||||
matches := 0
|
||||
for _, s := range latest {
|
||||
if s.Context == ctxName {
|
||||
matches++
|
||||
assert.Equal(t, legacyHash, s.ContextHash)
|
||||
assert.Equal(t, commitstatus.CommitStatusSuccess, s.State)
|
||||
}
|
||||
}
|
||||
assert.Equal(t, 1, matches)
|
||||
}
|
||||
|
||||
// TestCreateCommitStatus_UnnamedWorkflowUsesFileName: a workflow with no
|
||||
// non-blank `name:` uses the file name in the Context, not an empty
|
||||
// "/ job (event)" — covers both an omitted and a whitespace-only name.
|
||||
func TestCreateCommitStatus_UnnamedWorkflowUsesFileName(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
|
||||
branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
|
||||
|
||||
for _, tc := range []struct {
|
||||
workflowID string
|
||||
runID, jobID int64
|
||||
payload string
|
||||
}{
|
||||
{"unnamed.yaml", 99401, 99411, "on: push\n"},
|
||||
{"blank.yaml", 99402, 99412, "name: \" \"\non: push\n"},
|
||||
} {
|
||||
run := &actions_model.ActionRun{
|
||||
ID: tc.runID, Index: tc.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
|
||||
WorkflowID: tc.workflowID, CommitSHA: branch.CommitID,
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), run))
|
||||
job := &actions_model.ActionRunJob{
|
||||
ID: tc.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
|
||||
Name: "my-test", Status: actions_model.StatusWaiting,
|
||||
WorkflowPayload: []byte(tc.payload + `jobs:
|
||||
my-test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hi
|
||||
`),
|
||||
}
|
||||
require.NoError(t, db.Insert(t.Context(), job))
|
||||
require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job))
|
||||
|
||||
statuses := findCommitStatusesForContext(t, repo.ID, branch.CommitID, tc.workflowID+" / my-test (push)")
|
||||
require.Len(t, statuses, 1)
|
||||
assert.Equal(t, commitstatus.CommitStatusPending, statuses[0].State)
|
||||
}
|
||||
}
|
||||
|
||||
func findCommitStatusesForContext(t *testing.T, repoID int64, sha, context string) []*git_model.CommitStatus {
|
||||
t.Helper()
|
||||
|
||||
|
||||
@ -638,7 +638,7 @@ jobs:
|
||||
return false
|
||||
}
|
||||
if latestCommitStatuses[0].State == commitstatus.CommitStatusPending {
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context)
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0])
|
||||
return true
|
||||
}
|
||||
return false
|
||||
@ -680,14 +680,18 @@ func checkCommitStatusAndInsertFakeStatus(t *testing.T, repo *repo_model.Reposit
|
||||
assert.Len(t, latestCommitStatuses, 1)
|
||||
assert.Equal(t, commitstatus.CommitStatusPending, latestCommitStatuses[0].State)
|
||||
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context)
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0])
|
||||
}
|
||||
|
||||
func insertFakeStatus(t *testing.T, repo *repo_model.Repository, sha, targetURL, context string) {
|
||||
// insertFakeStatus inserts a success status that lands in the same dedupe
|
||||
// group as `prev` — the actions runner mixes the workflow file path into
|
||||
// ContextHash, so we must reuse it (rather than recomputing from Context).
|
||||
func insertFakeStatus(t *testing.T, repo *repo_model.Repository, sha string, prev *git_model.CommitStatus) {
|
||||
err := commitstatus_service.CreateCommitStatus(t.Context(), repo, user_model.NewActionsUser(), sha, &git_model.CommitStatus{
|
||||
State: commitstatus.CommitStatusSuccess,
|
||||
TargetURL: targetURL,
|
||||
Context: context,
|
||||
State: commitstatus.CommitStatusSuccess,
|
||||
TargetURL: prev.TargetURL,
|
||||
Context: prev.Context,
|
||||
ContextHash: prev.ContextHash,
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
@ -822,7 +826,7 @@ jobs:
|
||||
return false
|
||||
}
|
||||
if latestCommitStatuses[0].State == commitstatus.CommitStatusPending {
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context)
|
||||
insertFakeStatus(t, repo, sha, latestCommitStatuses[0])
|
||||
return true
|
||||
}
|
||||
return false
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user