tenseleyflow/shithub / 2bea772

Browse files

Fix PR mergeability for org repos

Authored by mfwolffe <wolffemf@dukes.jmu.edu>
SHA
2bea772b3a6d6dda58f869b000ed49ba5f9caaf1
Parents
17b19ff
Tree
3d2563e

4 changed files

StatusFile+-
M internal/web/handlers/repo/pull_review.go 2 2
M internal/web/handlers/repo/pulls.go 5 0
M internal/worker/jobs/pr_jobs.go 8 8
A internal/worker/jobs/pr_jobs_test.go 161 0
internal/web/handlers/repo/pull_review.gomodified
@@ -44,8 +44,8 @@ func (h *Handlers) reviewDeps() review.Deps {
4444
 	return review.Deps{Pool: h.d.Pool, Logger: h.d.Logger}
4545
 }
4646
 
47
-// kickMergeability enqueues a pr:mergeability job. Called after every
48
-// review-side write so the merge gate state reflects truth quickly.
47
+// kickMergeability enqueues a pr:mergeability job after review-side
48
+// writes and when an open PR view detects a still-unknown merge state.
4949
 func (h *Handlers) kickMergeability(r *http.Request, prID int64) {
5050
 	if _, err := worker.Enqueue(r.Context(), h.d.Pool, worker.KindPRMergeability,
5151
 		map[string]any{"pr_id": prID}, worker.EnqueueOptions{}); err != nil {
internal/web/handlers/repo/pulls.gomodified
@@ -391,6 +391,11 @@ func (h *Handlers) renderPullPage(w http.ResponseWriter, r *http.Request, tab st
391391
 			mergedByName = u.Username
392392
 		}
393393
 	}
394
+	if pr.IState == pullsdb.IssueStateOpen &&
395
+		pr.MergeableState == pullsdb.PrMergeableStateUnknown &&
396
+		pr.BaseOid != "" && pr.HeadOid != "" {
397
+		h.kickMergeability(r, pr.IID)
398
+	}
394399
 	checkGroups := h.pullCheckGroups(r.Context(), row.ID, pr.HeadOid)
395400
 	stats := h.pullStats(r.Context(), pr, checkGroups)
396401
 	data := map[string]any{
internal/worker/jobs/pr_jobs.gomodified
@@ -111,18 +111,18 @@ func resolveGitDirForPR(ctx context.Context, pool *pgxpool.Pool, rfs *storage.Re
111111
 		}
112112
 		return "", err
113113
 	}
114
-	repo, err := reposdb.New().GetRepoByID(ctx, pool, pr.HeadRepoID)
114
+	ownerRow, err := reposdb.New().GetRepoOwnerUsernameByID(ctx, pool, pr.HeadRepoID)
115115
 	if err != nil {
116
-		return "", err
117
-	}
118
-	if !repo.OwnerUserID.Valid {
119
-		return "", worker.PoisonError(fmt.Errorf("pr %d: repo has no owner user (org owners not yet supported here)", prID))
116
+		if errors.Is(err, pgx.ErrNoRows) {
117
+			return "", worker.PoisonError(fmt.Errorf("pr %d: repo %d not found", prID, pr.HeadRepoID))
118
+		}
119
+		return "", fmt.Errorf("load repo owner: %w", err)
120120
 	}
121
-	owner, err := usersdb.New().GetUserByID(ctx, pool, repo.OwnerUserID.Int64)
121
+	ownerSlug, err := ownerSlugString(ownerRow.OwnerUsername)
122122
 	if err != nil {
123
-		return "", err
123
+		return "", worker.PoisonError(fmt.Errorf("pr %d: repo owner slug: %w", prID, err))
124124
 	}
125
-	return rfs.RepoPath(owner.Username, repo.Name)
125
+	return rfs.RepoPath(ownerSlug, ownerRow.RepoName)
126126
 }
127127
 
128128
 // enqueuePRActionsTrigger fans out a workflow:trigger job for a PR
internal/worker/jobs/pr_jobs_test.goadded
@@ -0,0 +1,161 @@
1
+// SPDX-License-Identifier: AGPL-3.0-or-later
2
+
3
+package jobs
4
+
5
+import (
6
+	"context"
7
+	"encoding/json"
8
+	"io"
9
+	"log/slog"
10
+	"os"
11
+	"os/exec"
12
+	"path/filepath"
13
+	"strings"
14
+	"testing"
15
+
16
+	"github.com/jackc/pgx/v5/pgtype"
17
+
18
+	"github.com/tenseleyFlow/shithub/internal/auth/audit"
19
+	"github.com/tenseleyFlow/shithub/internal/auth/throttle"
20
+	"github.com/tenseleyFlow/shithub/internal/infra/storage"
21
+	"github.com/tenseleyFlow/shithub/internal/orgs"
22
+	"github.com/tenseleyFlow/shithub/internal/pulls"
23
+	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
24
+	"github.com/tenseleyFlow/shithub/internal/repos"
25
+	"github.com/tenseleyFlow/shithub/internal/testing/dbtest"
26
+	usersdb "github.com/tenseleyFlow/shithub/internal/users/sqlc"
27
+)
28
+
29
+const prJobsFixtureHash = "$argon2id$v=19$m=16384,t=1,p=1$" +
30
+	"AAAAAAAAAAAAAAAA$" +
31
+	"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
32
+
33
+func prJobsGitCmd(args ...string) *exec.Cmd {
34
+	//nolint:gosec
35
+	return exec.Command("git", args...)
36
+}
37
+
38
+func prJobsCommitOnBranch(t *testing.T, gitDir, branch, msg, file, contents string) string {
39
+	t.Helper()
40
+	wt := t.TempDir()
41
+	addArgs := []string{"-C", gitDir, "worktree", "add"}
42
+	if _, err := prJobsGitCmd("-C", gitDir, "show-ref", "--verify", "refs/heads/"+branch).CombinedOutput(); err != nil {
43
+		addArgs = append(addArgs, "-b", branch, wt)
44
+	} else {
45
+		addArgs = append(addArgs, wt, branch)
46
+	}
47
+	if out, err := prJobsGitCmd(addArgs...).CombinedOutput(); err != nil {
48
+		t.Fatalf("worktree add %s: %v (%s)", branch, err, out)
49
+	}
50
+	defer func() {
51
+		_ = prJobsGitCmd("-C", gitDir, "worktree", "remove", "--force", wt).Run()
52
+	}()
53
+
54
+	if err := os.MkdirAll(filepath.Dir(filepath.Join(wt, file)), 0o755); err != nil {
55
+		t.Fatalf("mkdir for %s: %v", file, err)
56
+	}
57
+	if err := os.WriteFile(filepath.Join(wt, file), []byte(contents), 0o644); err != nil { //nolint:gosec
58
+		t.Fatalf("write %s: %v", file, err)
59
+	}
60
+	for _, args := range [][]string{
61
+		{"-C", wt, "config", "user.name", "Alice"},
62
+		{"-C", wt, "config", "user.email", "alice@example.com"},
63
+		{"-C", wt, "add", "."},
64
+		{"-C", wt, "commit", "-m", msg},
65
+	} {
66
+		if out, err := prJobsGitCmd(args...).CombinedOutput(); err != nil {
67
+			t.Fatalf("%v: %v (%s)", args, err, out)
68
+		}
69
+	}
70
+	out, err := prJobsGitCmd("-C", wt, "rev-parse", "HEAD").Output()
71
+	if err != nil {
72
+		t.Fatalf("rev-parse HEAD: %v", err)
73
+	}
74
+	return strings.TrimSpace(string(out))
75
+}
76
+
77
+func TestPRMergeability_OrgOwnedRepoResolvesOwnerSlug(t *testing.T) {
78
+	t.Parallel()
79
+	ctx := context.Background()
80
+	pool := dbtest.NewTestDB(t)
81
+	rfs, err := storage.NewRepoFS(t.TempDir())
82
+	if err != nil {
83
+		t.Fatalf("NewRepoFS: %v", err)
84
+	}
85
+
86
+	uq := usersdb.New()
87
+	user, err := uq.CreateUser(ctx, pool, usersdb.CreateUserParams{
88
+		Username: "alice", DisplayName: "Alice", PasswordHash: prJobsFixtureHash,
89
+	})
90
+	if err != nil {
91
+		t.Fatalf("CreateUser: %v", err)
92
+	}
93
+	email, err := uq.CreateUserEmail(ctx, pool, usersdb.CreateUserEmailParams{
94
+		UserID: user.ID, Email: "alice@example.com", IsPrimary: true, Verified: true,
95
+	})
96
+	if err != nil {
97
+		t.Fatalf("CreateUserEmail: %v", err)
98
+	}
99
+	if err := uq.LinkUserPrimaryEmail(ctx, pool, usersdb.LinkUserPrimaryEmailParams{
100
+		ID: user.ID, PrimaryEmailID: pgtype.Int8{Int64: email.ID, Valid: true},
101
+	}); err != nil {
102
+		t.Fatalf("LinkUserPrimaryEmail: %v", err)
103
+	}
104
+
105
+	org, err := orgs.Create(ctx, orgs.Deps{Pool: pool}, orgs.CreateParams{
106
+		Slug: "acme", DisplayName: "Acme", CreatedByUserID: user.ID,
107
+	})
108
+	if err != nil {
109
+		t.Fatalf("orgs.Create: %v", err)
110
+	}
111
+	res, err := repos.Create(ctx, repos.Deps{
112
+		Pool: pool, RepoFS: rfs, Audit: audit.NewRecorder(), Limiter: throttle.NewLimiter(),
113
+	}, repos.Params{
114
+		OwnerOrgID: org.ID, OwnerSlug: string(org.Slug), ActorUserID: user.ID,
115
+		Name: "demo", Visibility: "public", InitReadme: true,
116
+	})
117
+	if err != nil {
118
+		t.Fatalf("repos.Create: %v", err)
119
+	}
120
+	gitDir, err := rfs.RepoPath(string(org.Slug), res.Repo.Name)
121
+	if err != nil {
122
+		t.Fatalf("RepoPath: %v", err)
123
+	}
124
+	prJobsCommitOnBranch(t, gitDir, "feature", "add feature", "feature.txt", "feature\n")
125
+
126
+	pr, err := pulls.Create(ctx, pulls.Deps{
127
+		Pool:   pool,
128
+		Logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
129
+	}, pulls.CreateParams{
130
+		RepoID:       res.Repo.ID,
131
+		AuthorUserID: user.ID,
132
+		Title:        "Add feature",
133
+		BaseRef:      "trunk",
134
+		HeadRef:      "feature",
135
+		GitDir:       gitDir,
136
+	})
137
+	if err != nil {
138
+		t.Fatalf("pulls.Create: %v", err)
139
+	}
140
+
141
+	handler := PRMergeability(PRJobsDeps{
142
+		Pool:   pool,
143
+		RepoFS: rfs,
144
+		Logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
145
+	})
146
+	payload, _ := json.Marshal(PRMergeabilityPayload{PRID: pr.PullRequest.IssueID})
147
+	if err := handler(ctx, payload); err != nil {
148
+		t.Fatalf("PRMergeability: %v", err)
149
+	}
150
+
151
+	got, err := pullsdb.New().GetPullRequestByIssueID(ctx, pool, pr.PullRequest.IssueID)
152
+	if err != nil {
153
+		t.Fatalf("GetPullRequestByIssueID: %v", err)
154
+	}
155
+	if got.MergeableState != pullsdb.PrMergeableStateClean {
156
+		t.Fatalf("mergeable_state = %s, want clean", got.MergeableState)
157
+	}
158
+	if !got.Mergeable.Valid || !got.Mergeable.Bool {
159
+		t.Fatalf("mergeable = %+v, want true", got.Mergeable)
160
+	}
161
+}