Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/lib/api-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lib/pulls/pr-handoff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PullRequestDetail> => ({
Expand Down
9 changes: 8 additions & 1 deletion internal/project/pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
89 changes: 88 additions & 1 deletion internal/project/prchecks.go
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
60 changes: 59 additions & 1 deletion internal/project/prchecks_test.go
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
}
}
6 changes: 6 additions & 0 deletions native/cmd/lich-native/pulls_panels.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
})
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/en/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/es/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/ja/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"ruleNote": "{base} にはルールセットが適用されています: すべてのコミットについて、{clauses}。"
},
"pullsChecks": {
"none": "報告されたチェックはありません。"
"none": "報告されたチェックはありません。",
"required": "必須"
},
"pullsCommits": {
"none": "コミットはありません。"
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/ko/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"ruleNote": "{base}에 규칙 세트가 적용됩니다. 모든 커밋에서 {clauses}."
},
"pullsChecks": {
"none": "보고된 체크가 없습니다."
"none": "보고된 체크가 없습니다.",
"required": "필수"
},
"pullsCommits": {
"none": "커밋이 없습니다."
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/pt-br/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/ru/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"ruleNote": "Для {base} действует набор правил. Каждый коммит: {clauses}."
},
"pullsChecks": {
"none": "Проверок нет."
"none": "Проверок нет.",
"required": "Обязательный"
},
"pullsCommits": {
"none": "Коммитов нет."
Expand Down
3 changes: 2 additions & 1 deletion native/i18n/locales/zh-cn/pulls.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"ruleNote": "规则集约束着 {base}:对每个提交,{clauses}。"
},
"pullsChecks": {
"none": "没有检查结果。"
"none": "没有检查结果。",
"required": "必需"
},
"pullsCommits": {
"none": "没有提交。"
Expand Down
1 change: 1 addition & 0 deletions native/lichclient/pulls.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading