diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ea439fd..9d284c6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Required checks are labelled.** In the native window, a pull request's + Checks tab marks the checks that gate its merge with Required, as GitHub + does. + ### Changed - **lich lives at `lichdotdev/lich`.** The repository moved to the diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 5b85b944..7fd80c9a 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -120,6 +120,8 @@ export interface CheckItem { /** gh's ISO timestamps; "" while a check has not started or finished. */ startedAt: string completedAt: string + /** The check gates the merge, as GitHub's own checks list labels Required. */ + required: boolean } /** internal/project.PRCommit — one commit of the PR, for the Commits tab. */ diff --git a/frontend/src/lib/pulls/pr-handoff.test.ts b/frontend/src/lib/pulls/pr-handoff.test.ts index da59503f..af49d362 100644 --- a/frontend/src/lib/pulls/pr-handoff.test.ts +++ b/frontend/src/lib/pulls/pr-handoff.test.ts @@ -33,6 +33,7 @@ const run = (name: string, state: CheckItem["state"] = "failed"): CheckItem => ( url: `https://github.com/o/l/runs/${name}`, startedAt: "", completedAt: "", + required: false, }) const red = (names: string[]): Partial => ({ diff --git a/internal/project/pr.go b/internal/project/pr.go index 3dc20ab0..5b7f2b0c 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -213,7 +213,14 @@ func (s *Service) PullRequestDetail(path string, number int) (*PRDetail, error) // The OPEN-only gate belongs to the branch lookup alone. Naming a number is // asking for that pull request, whatever became of it — the list can show // merged and closed ones, and a row that cannot be opened is not a row. - return parsePRDetail(out, number == 0) + detail, err := parsePRDetail(out, number == 0) + if err != nil || detail == nil || len(detail.CheckRuns) == 0 { + return detail, err + } + if err := s.markRequiredChecks(path, detail.Number, detail.CheckRuns); err != nil { + return nil, err + } + return detail, nil } // parsePRDetail decodes gh's JSON and reduces the check rollup. With openOnly it diff --git a/internal/project/prchecks.go b/internal/project/prchecks.go index fed730a0..f1df4bb3 100644 --- a/internal/project/prchecks.go +++ b/internal/project/prchecks.go @@ -1,6 +1,10 @@ package project -import "sort" +import ( + "encoding/json" + "fmt" + "sort" +) // gh reports a pull request's CI as a mixed array of two unrelated shapes. This // file is the whole translation from that array into the two things the screen @@ -27,6 +31,9 @@ type CheckItem struct { URL string `json:"url"` // where to read the run; "" when gh reports none StartedAt string `json:"startedAt"` CompletedAt string `json:"completedAt"` + // Required is GitHub's own answer to "does this check gate the merge", the + // same verdict its checks list labels Required. + Required bool `json:"required"` } // checkItem is one statusCheckRollup entry. gh emits two shapes: a CheckRun @@ -128,3 +135,83 @@ func toCheckItems(items []checkItem) []CheckItem { }) return out } + +// requiredChecksQuery asks GitHub which of the head commit's checks gate this +// pull request's merge. gh's `pr view --json statusCheckRollup` does not carry +// isRequired, which takes the pull request as an argument. The aliases fold a +// StatusContext into a CheckRun's field names. +// ponytail: first 100 checks only; a check past that never reads Required. +const requiredChecksQuery = `query($owner:String!,$repo:String!,$number:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$number){ + commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{ + ... on CheckRun{name detailsUrl isRequired(pullRequestNumber:$number)} + ... on StatusContext{name:context detailsUrl:targetUrl isRequired(pullRequestNumber:$number)} + }}}}}} + } + } +}` + +// markRequiredChecks flags the checks that gate the merge of pull request +// number. Checks are matched by name and URL: a name alone repeats across +// workflows ("test" in two of them), and the URL names the run. +func (s *Service) markRequiredChecks(path string, number int, checks []CheckItem) error { + out, err := s.gh(prReadTimeout, path, "api", "graphql", + "-F", "owner={owner}", "-F", "repo={repo}", + "-F", fmt.Sprintf("number=%d", number), + "-f", "query="+requiredChecksQuery) + if err != nil { + return err + } + required, err := parseRequiredChecks(out) + if err != nil { + return err + } + for i := range checks { + checks[i].Required = required[checkKey(checks[i].Name, checks[i].URL)] + } + return nil +} + +func checkKey(name, url string) string { return name + "\n" + url } + +func parseRequiredChecks(out []byte) (map[string]bool, error) { + var payload struct { + Data struct { + Repository struct { + PullRequest struct { + Commits struct { + Nodes []struct { + Commit struct { + StatusCheckRollup *struct { + Contexts struct { + Nodes []struct { + Name string `json:"name"` + DetailsURL string `json:"detailsUrl"` + IsRequired bool `json:"isRequired"` + } `json:"nodes"` + } `json:"contexts"` + } `json:"statusCheckRollup"` + } `json:"commit"` + } `json:"nodes"` + } `json:"commits"` + } `json:"pullRequest"` + } `json:"repository"` + } `json:"data"` + } + if err := json.Unmarshal(out, &payload); err != nil { + return nil, errUnreadableAnswer(err) + } + required := map[string]bool{} + for _, c := range payload.Data.Repository.PullRequest.Commits.Nodes { + if c.Commit.StatusCheckRollup == nil { + continue + } + for _, n := range c.Commit.StatusCheckRollup.Contexts.Nodes { + if n.IsRequired { + required[checkKey(n.Name, n.DetailsURL)] = true + } + } + } + return required, nil +} diff --git a/internal/project/prchecks_test.go b/internal/project/prchecks_test.go index 0975ec67..cf1b1178 100644 --- a/internal/project/prchecks_test.go +++ b/internal/project/prchecks_test.go @@ -1,6 +1,11 @@ package project -import "testing" +import ( + "errors" + "maps" + "slices" + "testing" +) // TestIsFailureConclusion pins every conclusion gh reports, both halves of the // list. It is an allow-list of failures, so a literal dropped from it does not @@ -31,3 +36,56 @@ func TestIsFailureConclusion(t *testing.T) { }) } } + +// TestPullRequestDetailMarksRequiredChecks pins Required to GitHub's isRequired, +// matched per run: "test" runs in two workflows and only the URL tells them +// apart. +func TestPullRequestDetailMarksRequiredChecks(t *testing.T) { + t.Parallel() + view := []byte(`{"number":818,"state":"OPEN","statusCheckRollup":[ + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"SUCCESS","workflowName":"ci","detailsUrl":"https://x/ci"}, + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"SUCCESS","workflowName":"native","detailsUrl":"https://x/native"}, + {"__typename":"StatusContext","context":"deploy","state":"SUCCESS","targetUrl":"https://x/deploy"} + ]}`) + required := []byte(`{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{"nodes":[ + {"name":"test","detailsUrl":"https://x/ci","isRequired":true}, + {"name":"test","detailsUrl":"https://x/native","isRequired":false}, + {"name":"deploy","detailsUrl":"https://x/deploy","isRequired":true} + ]}}}}]}}}}}`) + gh := &fakeGH{outs: [][]byte{view, required}} + pr, err := withGH(gh).PullRequestDetail("/repo", 0) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + got := map[string]bool{} + for _, c := range pr.CheckRuns { + got[c.URL] = c.Required + } + want := map[string]bool{"https://x/ci": true, "https://x/native": false, "https://x/deploy": true} + if !maps.Equal(got, want) { + t.Errorf("required = %v, want %v", got, want) + } + if !slices.Contains(gh.history[1], "number=818") { + t.Errorf("lookup args = %v, want the PR number gh resolved", gh.history[1]) + } +} + +func TestPullRequestDetailRequiredLookupFailure(t *testing.T) { + t.Parallel() + view := []byte(`{"number":7,"state":"OPEN","statusCheckRollup":[{"name":"test","status":"IN_PROGRESS"}]}`) + gh := &fakeGH{outs: [][]byte{view, nil}, errs: []error{nil, errors.New("gh is not signed in to GitHub.")}} + if _, err := withGH(gh).PullRequestDetail("/repo", 0); err == nil { + t.Fatal("expected the lookup's error") + } +} + +func TestPullRequestDetailWithoutChecksSkipsTheLookup(t *testing.T) { + t.Parallel() + gh := &fakeGH{out: []byte(`{"number":7,"state":"OPEN"}`)} + if _, err := withGH(gh).PullRequestDetail("/repo", 0); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gh.calls != 1 { + t.Errorf("gh calls = %d, want 1", gh.calls) + } +} diff --git a/native/cmd/lich-native/pulls_panels.go b/native/cmd/lich-native/pulls_panels.go index f3fbbfcb..5bb56aa7 100644 --- a/native/cmd/lich-native/pulls_panels.go +++ b/native/cmd/lich-native/pulls_panels.go @@ -375,6 +375,12 @@ func (v *pullView) checkRows(gtx C, th *ui.Theme, d lichclient.PRDetail, act *pu }), ) }), + layout.Rigid(func(gtx C) D { + if !c.Required { + return D{} + } + return layout.Inset{Right: ui.Space(3)}.Layout(gtx, paletteChip(th, i18n.T("pulls.pullsChecks.required"), th.MutedForeground)) + }), layout.Rigid(th.Text(ui.TextXS, checkDuration(c.StartedAt, c.CompletedAt, now)).Muted().Layout), ) }) diff --git a/native/i18n/locales/en/pulls.json b/native/i18n/locales/en/pulls.json index 41312d35..7e6adb54 100644 --- a/native/i18n/locales/en/pulls.json +++ b/native/i18n/locales/en/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "A ruleset governs {base}: every commit {clauses}." }, "pullsChecks": { - "none": "No checks reported." + "none": "No checks reported.", + "required": "Required" }, "pullsCommits": { "none": "No commits." diff --git a/native/i18n/locales/es/pulls.json b/native/i18n/locales/es/pulls.json index 8dc863c7..3d90b7cc 100644 --- a/native/i18n/locales/es/pulls.json +++ b/native/i18n/locales/es/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Un conjunto de reglas gobierna {base}: cada commit {clauses}." }, "pullsChecks": { - "none": "No se informó ninguna comprobación." + "none": "No se informó ninguna comprobación.", + "required": "Obligatorio" }, "pullsCommits": { "none": "No hay commits." diff --git a/native/i18n/locales/ja/pulls.json b/native/i18n/locales/ja/pulls.json index 160997f0..c283b393 100644 --- a/native/i18n/locales/ja/pulls.json +++ b/native/i18n/locales/ja/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "{base} にはルールセットが適用されています: すべてのコミットについて、{clauses}。" }, "pullsChecks": { - "none": "報告されたチェックはありません。" + "none": "報告されたチェックはありません。", + "required": "必須" }, "pullsCommits": { "none": "コミットはありません。" diff --git a/native/i18n/locales/ko/pulls.json b/native/i18n/locales/ko/pulls.json index fc2b19cb..7ab46bb4 100644 --- a/native/i18n/locales/ko/pulls.json +++ b/native/i18n/locales/ko/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "{base}에 규칙 세트가 적용됩니다. 모든 커밋에서 {clauses}." }, "pullsChecks": { - "none": "보고된 체크가 없습니다." + "none": "보고된 체크가 없습니다.", + "required": "필수" }, "pullsCommits": { "none": "커밋이 없습니다." diff --git a/native/i18n/locales/pt-br/pulls.json b/native/i18n/locales/pt-br/pulls.json index e23cd815..e7b446ea 100644 --- a/native/i18n/locales/pt-br/pulls.json +++ b/native/i18n/locales/pt-br/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Um ruleset governa {base}: em todo commit, {clauses}." }, "pullsChecks": { - "none": "Nenhuma verificação reportada." + "none": "Nenhuma verificação reportada.", + "required": "Obrigatório" }, "pullsCommits": { "none": "Nenhum commit." diff --git a/native/i18n/locales/ru/pulls.json b/native/i18n/locales/ru/pulls.json index 8ab9a929..dccffe9e 100644 --- a/native/i18n/locales/ru/pulls.json +++ b/native/i18n/locales/ru/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Для {base} действует набор правил. Каждый коммит: {clauses}." }, "pullsChecks": { - "none": "Проверок нет." + "none": "Проверок нет.", + "required": "Обязательный" }, "pullsCommits": { "none": "Коммитов нет." diff --git a/native/i18n/locales/zh-cn/pulls.json b/native/i18n/locales/zh-cn/pulls.json index 354698e4..32950f8f 100644 --- a/native/i18n/locales/zh-cn/pulls.json +++ b/native/i18n/locales/zh-cn/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "规则集约束着 {base}:对每个提交,{clauses}。" }, "pullsChecks": { - "none": "没有检查结果。" + "none": "没有检查结果。", + "required": "必需" }, "pullsCommits": { "none": "没有提交。" diff --git a/native/lichclient/pulls.go b/native/lichclient/pulls.go index 1bbfb563..57e29e5f 100644 --- a/native/lichclient/pulls.go +++ b/native/lichclient/pulls.go @@ -28,6 +28,7 @@ type CheckItem struct { URL string `json:"url"` StartedAt string `json:"startedAt"` CompletedAt string `json:"completedAt"` + Required bool `json:"required"` } // PRSummary mirrors project.PRSummary: one row of the list.