diff --git a/docs/configuration.md b/docs/configuration.md index 2ef66f7..7408544 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -135,7 +135,7 @@ Filters = ["FilterMyPRs"] # Explicitly add this filter The `ProjectListWorkflow` pulls information from Jira to build a realtime list of all PRs which are linked to children cards of the Jira epic given in the config. -Each workflow is tied to a single github repository, if you want multiple repos per project, create two workflows and have them use the same SectionTitle. +A project often spans more than one repository, so `Repos` takes a list, just like the other workflow types. Every repo in the list is checked against the epic's linked PRs, each repo's PRs are evaluated independently, and they all land in the same section — one workflow covers the whole project. If `Repos` is omitted the workflow falls back to the root-level `Repos` list. ```bash export JIRA_API_TOKEN="Jira API Token" @@ -148,10 +148,23 @@ JiraDomain="https://your-company.atlassain.net" [[Workflows]] WorkflowType = "ProjectListWorkflow" Name = "Project - Example" +Repos = ["C-Hipple/diff-lsp", "C-Hipple/code-review-server"] +SectionTitle = "Diff LSP Upgrade Project" +JiraEpic = "BOARD-123" # the epic key +``` + +Repos are matched on the full `owner/repo` pair, so a project can span repos that share a short name under different owners. PRs linked to the epic from a repo outside the list are ignored. + +The singular `Owner`/`Repo` pair is still accepted for a single-repo project, but `Repos` is preferred: + +```toml +[[Workflows]] +WorkflowType = "ProjectListWorkflow" +Name = "Project - Single Repo" Owner = "C-Hipple" Repo = "diff-lsp" SectionTitle = "Diff LSP Upgrade Project" -JiraEpic = "BOARD-123" # the epic key +JiraEpic = "BOARD-123" ``` ## Release Checking diff --git a/jira/jira.go b/jira/jira.go index 76e6704..4e48ff6 100644 --- a/jira/jira.go +++ b/jira/jira.go @@ -43,9 +43,44 @@ func getAuth() (string, string) { return jiraEmail, token } +// RepoRef identifies a GitHub repository by owner and short name. A project can +// span several repos, and two of them may share a short name under different +// owners, so PRs are matched and grouped on the pair rather than on the name. +type RepoRef struct { + Owner string + Repo string +} + +func (r RepoRef) FullName() string { + return fmt.Sprintf("%s/%s", r.Owner, r.Repo) +} + +// normalized lowercases the ref for comparison; GitHub owner and repo names are +// case-insensitive, and Jira reports whatever casing the PR was linked with. +func (r RepoRef) normalized() RepoRef { + return RepoRef{Owner: strings.ToLower(r.Owner), Repo: strings.ToLower(r.Repo)} +} + +// targetSet maps a normalized ref back to the ref as it was configured, so +// callers get keys they can hand straight to the GitHub API. +type targetSet map[RepoRef]RepoRef + +func newTargetSet(refs []RepoRef) targetSet { + targets := make(targetSet, len(refs)) + for _, ref := range refs { + targets[ref.normalized()] = ref + } + return targets +} + +func (t targetSet) lookup(ref RepoRef) (RepoRef, bool) { + configured, ok := t[ref.normalized()] + return configured, ok +} + // GetProjectPRKeys returns the PR numbers for every target repo under a JIRA epic, -// keyed by the repo short name (e.g. "code-review-server"). -func GetProjectPRKeys(domain string, epicKey string, repo_names []string) map[string][]int { +// keyed by the repo they belong to. +func GetProjectPRKeys(domain string, epicKey string, repos []RepoRef) map[RepoRef][]int { if !strings.HasSuffix(domain, "/") { domain += "/" } @@ -60,7 +95,7 @@ func GetProjectPRKeys(domain string, epicKey string, repo_names []string) map[st req, err := http.NewRequest("GET", searchURL, nil) if err != nil { slog.Error("Error creating request", "error", err) - return map[string][]int{} + return map[RepoRef][]int{} } req.URL.RawQuery = params.Encode() @@ -71,7 +106,7 @@ func GetProjectPRKeys(domain string, epicKey string, repo_names []string) map[st resp, err := client.Do(req) if err != nil { slog.Error("Error making request", "error", err) - return map[string][]int{} + return map[RepoRef][]int{} } defer resp.Body.Close() @@ -81,31 +116,44 @@ func GetProjectPRKeys(domain string, epicKey string, repo_names []string) map[st body := make([]byte, 1024) n, _ := resp.Body.Read(body) // We ignore the error here. slog.Error("JIRA response body", "content", string(body[:n])) - return map[string][]int{} + return map[RepoRef][]int{} } var data JiraSearchResponse if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { slog.Error("Error decoding JSON", "error", err) - return map[string][]int{} + return map[RepoRef][]int{} } - return processIssues(domain, data, repo_names) + return processIssues(domain, data, repos) } type issuePR struct { - repo string - num int + ref RepoRef + num int } -func processIssues(domain string, data JiraSearchResponse, target_repos []string) map[string][]int { - // Returns a map of repo short name -> list of PR numbers for that repo. - - targets := make(map[string]struct{}, len(target_repos)) - for _, r := range target_repos { - targets[r] = struct{}{} +// parsePRURL pulls the repo and PR number out of a GitHub pull request URL of +// the form https://///pull/. +func parsePRURL(prURL string) (RepoRef, int, error) { + parts := strings.Split(strings.TrimSuffix(prURL, "/"), "/") + if len(parts) < 4 { + return RepoRef{}, 0, fmt.Errorf("unexpected PR URL %q", prURL) + } + num, err := strconv.Atoi(parts[len(parts)-1]) + if err != nil { + return RepoRef{}, 0, fmt.Errorf("failed to convert PR number %q to int", parts[len(parts)-1]) } + return RepoRef{Owner: parts[len(parts)-4], Repo: parts[len(parts)-3]}, num, nil +} + +func processIssues(domain string, data JiraSearchResponse, target_repos []RepoRef) map[RepoRef][]int { + // Returns a map of repo -> list of PR numbers for that repo. PRs linked to + // repos outside the target list are dropped, so a project spanning several + // repos gets each repo's PRs grouped independently. + + targets := newTargetSet(target_repos) var wg sync.WaitGroup @@ -133,19 +181,19 @@ func processIssues(domain string, data JiraSearchResponse, target_repos []string return } - urlParts := strings.Split(pr.URL, "/") - repo := urlParts[len(urlParts)-3] - if _, ok := targets[repo]; !ok { - errChan <- fmt.Errorf("Issue PR is for a separate repo: %s", repo) + ref, num, err := parsePRURL(pr.URL) + if err != nil { + errChan <- fmt.Errorf("issue %s: %w", issue.ID, err) return } - prNum := urlParts[len(urlParts)-1] - num, err := strconv.Atoi(prNum) - if err != nil { - errChan <- fmt.Errorf("Failed to convert prNum %s to int", prNum) + // Match on owner and repo together: a project may span repos that + // share a short name under different owners. + configured, ok := targets.lookup(ref) + if !ok { + errChan <- fmt.Errorf("Issue PR is for a separate repo: %s", ref.FullName()) return } - resultsChan <- issuePR{repo: repo, num: num} + resultsChan <- issuePR{ref: configured, num: num} }(issue) } @@ -153,9 +201,9 @@ func processIssues(domain string, data JiraSearchResponse, target_repos []string close(errChan) close(resultsChan) - out := make(map[string][]int) + out := make(map[RepoRef][]int) for r := range resultsChan { - out[r.repo] = append(out[r.repo], r.num) + out[r.ref] = append(out[r.ref], r.num) } return out } diff --git a/jira/jira_test.go b/jira/jira_test.go new file mode 100644 index 0000000..5bad55f --- /dev/null +++ b/jira/jira_test.go @@ -0,0 +1,218 @@ +package jira + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "sort" + "strings" + "testing" +) + +func TestParsePRURL(t *testing.T) { + tests := []struct { + name string + url string + wantRef RepoRef + wantNum int + wantError bool + }{ + { + name: "standard github PR url", + url: "https://github.com/C-Hipple/code-review-server/pull/197", + wantRef: RepoRef{Owner: "C-Hipple", Repo: "code-review-server"}, + wantNum: 197, + }, + { + name: "trailing slash", + url: "https://github.com/C-Hipple/diff-lsp/pull/12/", + wantRef: RepoRef{Owner: "C-Hipple", Repo: "diff-lsp"}, + wantNum: 12, + }, + { + name: "enterprise host", + url: "https://git.example.com/platform/api/pull/8", + wantRef: RepoRef{Owner: "platform", Repo: "api"}, + wantNum: 8, + }, + { + name: "too short to parse", + url: "pull/3", + wantError: true, + }, + { + name: "non-numeric PR number", + url: "https://github.com/owner/repo/pull/abc", + wantError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ref, num, err := parsePRURL(tt.url) + if tt.wantError { + if err == nil { + t.Fatalf("parsePRURL(%q) = %v, %d, want error", tt.url, ref, num) + } + return + } + if err != nil { + t.Fatalf("parsePRURL(%q) returned unexpected error: %v", tt.url, err) + } + if ref != tt.wantRef { + t.Errorf("ref = %v, want %v", ref, tt.wantRef) + } + if num != tt.wantNum { + t.Errorf("num = %d, want %d", num, tt.wantNum) + } + }) + } +} + +func TestTargetSetLookup(t *testing.T) { + configured := RepoRef{Owner: "C-Hipple", Repo: "Code-Review-Server"} + targets := newTargetSet([]RepoRef{configured, {Owner: "other-org", Repo: "code-review-server"}}) + + // Jira reports whatever casing the PR was linked with; the configured ref + // (with its original casing) is what comes back so it can be handed to the + // GitHub API. + got, ok := targets.lookup(RepoRef{Owner: "c-hipple", Repo: "code-review-server"}) + if !ok { + t.Fatal("expected case-insensitive match") + } + if got != configured { + t.Errorf("lookup returned %v, want the configured ref %v", got, configured) + } + + // Same short name, different owner, resolves to the other entry. + got, ok = targets.lookup(RepoRef{Owner: "other-org", Repo: "code-review-server"}) + if !ok { + t.Fatal("expected match for the second owner") + } + if want := (RepoRef{Owner: "other-org", Repo: "code-review-server"}); got != want { + t.Errorf("lookup returned %v, want %v", got, want) + } + + if _, ok := targets.lookup(RepoRef{Owner: "C-Hipple", Repo: "diff-lsp"}); ok { + t.Error("expected no match for a repo outside the target list") + } +} + +// devStatusServer serves the Jira dev-status endpoint, mapping issue ID to the +// PR URL linked from that issue. +func devStatusServer(t *testing.T, prsByIssue map[string]string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.Contains(r.URL.Path, "dev-status") { + http.NotFound(w, r) + return + } + issueID := r.URL.Query().Get("issueId") + prURL, ok := prsByIssue[issueID] + if !ok { + json.NewEncoder(w).Encode(DevStatusResponse{}) + return + } + json.NewEncoder(w).Encode(DevStatusResponse{ + Detail: []DevDetails{{ + PullRequests: []JiraPullRequestIdentifier{{URL: prURL, Status: "OPEN"}}, + }}, + }) + })) +} + +func issues(ids ...string) JiraSearchResponse { + resp := JiraSearchResponse{} + for _, id := range ids { + resp.Issues = append(resp.Issues, Issue{ID: id}) + } + return resp +} + +func TestProcessIssuesGroupsPRsAcrossRepos(t *testing.T) { + server := devStatusServer(t, map[string]string{ + "1": "https://github.com/C-Hipple/code-review-server/pull/10", + "2": "https://github.com/C-Hipple/diff-lsp/pull/20", + "3": "https://github.com/C-Hipple/code-review-server/pull/11", + // Linked to a repo the project doesn't span; must be dropped. + "4": "https://github.com/C-Hipple/unrelated/pull/99", + // No PR linked at all. + "5": "", + }) + defer server.Close() + + crs := RepoRef{Owner: "C-Hipple", Repo: "code-review-server"} + diffLsp := RepoRef{Owner: "C-Hipple", Repo: "diff-lsp"} + + got := processIssues(server.URL, issues("1", "2", "3", "4", "5"), []RepoRef{crs, diffLsp}) + + want := map[RepoRef][]int{ + crs: {10, 11}, + diffLsp: {20}, + } + for ref := range got { + sort.Ints(got[ref]) + } + if !reflect.DeepEqual(got, want) { + t.Errorf("processIssues() = %v, want %v", got, want) + } +} + +func TestProcessIssuesDistinguishesSameRepoNameAcrossOwners(t *testing.T) { + server := devStatusServer(t, map[string]string{ + "1": "https://github.com/org-a/service/pull/1", + "2": "https://github.com/org-b/service/pull/2", + }) + defer server.Close() + + orgA := RepoRef{Owner: "org-a", Repo: "service"} + orgB := RepoRef{Owner: "org-b", Repo: "service"} + + got := processIssues(server.URL, issues("1", "2"), []RepoRef{orgA, orgB}) + + want := map[RepoRef][]int{ + orgA: {1}, + orgB: {2}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("processIssues() = %v, want %v", got, want) + } +} + +func TestProcessIssuesExcludesUnconfiguredOwner(t *testing.T) { + server := devStatusServer(t, map[string]string{ + "1": "https://github.com/fork-org/service/pull/1", + }) + defer server.Close() + + got := processIssues(server.URL, issues("1"), []RepoRef{{Owner: "org-a", Repo: "service"}}) + + if len(got) != 0 { + t.Errorf("processIssues() = %v, want no results for an owner outside the target list", got) + } +} + +func TestRepoRefFullName(t *testing.T) { + ref := RepoRef{Owner: "C-Hipple", Repo: "code-review-server"} + if got, want := ref.FullName(), "C-Hipple/code-review-server"; got != want { + t.Errorf("FullName() = %q, want %q", got, want) + } +} + +// Guard the URL shape processIssues depends on: the dev-status request must +// carry the issue ID as a query parameter. +func TestGetDevURLCarriesIssueID(t *testing.T) { + raw := getDevURL("https://example.atlassian.net", "12345") + parsed, err := url.Parse(raw) + if err != nil { + t.Fatalf("getDevURL produced an unparseable URL %q: %v", raw, err) + } + if got := parsed.Query().Get("issueId"); got != "12345" { + t.Errorf("issueId = %q, want %q", got, "12345") + } + if got := parsed.Query().Get("dataType"); got != "pullrequest" { + t.Errorf("dataType = %q, want %q", got, "pullrequest") + } +} diff --git a/server/config_rpc_test.go b/server/config_rpc_test.go index c91080a..e899fe4 100644 --- a/server/config_rpc_test.go +++ b/server/config_rpc_test.go @@ -2,8 +2,11 @@ package server import ( "crs/config" + "crs/workflows" "os" "path/filepath" + "reflect" + "slices" "strings" "testing" ) @@ -116,6 +119,83 @@ func TestUpdateConfigSavesValidWorkflows(t *testing.T) { } } +// A project spanning several repos is configured as one workflow with a Repos +// list, so the whole list has to survive the client round-trip: through +// UpdateConfig, onto disk, and back out of the reloaded config. +func TestUpdateConfigSavesMultiRepoProjectList(t *testing.T) { + path := useTempConfig(t, testConfigTOML) + + h := &RPCHandler{} + newWorkflows := []config.RawWorkflow{{ + WorkflowType: "ProjectListWorkflow", + Name: "Project - Multi", + SectionTitle: "Multi Repo Project", + JiraEpic: "BOARD-123", + Repos: []string{" C-Hipple/code-review-server ", "", "C-Hipple/diff-lsp"}, + }} + reply := &UpdateConfigReply{} + if err := h.UpdateConfig(&UpdateConfigArgs{Workflows: &newWorkflows}, reply); err != nil { + t.Fatalf("UpdateConfig() error = %v", err) + } + + if !reply.Okay { + t.Fatalf("expected a multi-repo ProjectListWorkflow to be accepted, got %v", reply.Errors) + } + wantRepos := []string{"C-Hipple/code-review-server", "C-Hipple/diff-lsp"} + if got := reply.Config.Workflows[0].Repos; !reflect.DeepEqual(got, wantRepos) { + t.Errorf("expected blank entries dropped and the rest trimmed, got %v", got) + } + if got := config.C().RawWorkflows[0].Repos; !reflect.DeepEqual(got, wantRepos) { + t.Errorf("expected both repos in the reloaded config, got %v", got) + } + + written, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read written config: %v", err) + } + for _, repo := range wantRepos { + if !strings.Contains(string(written), repo) { + t.Errorf("expected %q on disk, got:\n%s", repo, written) + } + } + + // And the saved config must still build into a workflow carrying every repo. + built := workflows.MatchWorkflows(config.C().RawWorkflows, &[]string{}, "https://example.atlassian.net") + if len(built) != 1 { + t.Fatalf("expected the saved workflow to build, got %d", len(built)) + } + wf, ok := built[0].(workflows.ProjectListWorkflow) + if !ok { + t.Fatalf("expected a ProjectListWorkflow, got %T", built[0]) + } + if !reflect.DeepEqual(wf.Repos, wantRepos) { + t.Errorf("built workflow Repos = %v, want %v", wf.Repos, wantRepos) + } +} + +// Clients build their workflow editor from the registry this handler serves, so +// ProjectListWorkflow has to advertise Repos for the field to be offered at all. +func TestGetConfigAdvertisesReposForProjectList(t *testing.T) { + useTempConfig(t, testConfigTOML) + + h := &RPCHandler{} + reply := &GetConfigReply{} + if err := h.GetConfig(&GetConfigArgs{}, reply); err != nil { + t.Fatalf("GetConfig() error = %v", err) + } + + for _, wt := range reply.WorkflowTypes { + if wt.Name != "ProjectListWorkflow" { + continue + } + if !slices.Contains(wt.OptionalFields, "Repos") { + t.Errorf("expected ProjectListWorkflow to offer Repos, got %v", wt.OptionalFields) + } + return + } + t.Error("ProjectListWorkflow missing from the workflow type registry") +} + func TestUpdateConfigRejectsInvalidWorkflows(t *testing.T) { path := useTempConfig(t, testConfigTOML) diff --git a/workflows/project_list_test.go b/workflows/project_list_test.go new file mode 100644 index 0000000..90618cc --- /dev/null +++ b/workflows/project_list_test.go @@ -0,0 +1,146 @@ +package workflows + +import ( + "crs/config" + "crs/jira" + "reflect" + "testing" +) + +func TestResolveRepoRefs(t *testing.T) { + tests := []struct { + name string + entries []string + want []jira.RepoRef + }{ + { + name: "single repo", + entries: []string{"C-Hipple/code-review-server"}, + want: []jira.RepoRef{{Owner: "C-Hipple", Repo: "code-review-server"}}, + }, + { + name: "multiple repos keep configured order", + entries: []string{"C-Hipple/code-review-server", "C-Hipple/diff-lsp"}, + want: []jira.RepoRef{ + {Owner: "C-Hipple", Repo: "code-review-server"}, + {Owner: "C-Hipple", Repo: "diff-lsp"}, + }, + }, + { + name: "repos spanning owners", + entries: []string{"org-a/service", "org-b/service"}, + want: []jira.RepoRef{ + {Owner: "org-a", Repo: "service"}, + {Owner: "org-b", Repo: "service"}, + }, + }, + { + name: "surrounding whitespace is trimmed", + entries: []string{" C-Hipple/diff-lsp "}, + want: []jira.RepoRef{{Owner: "C-Hipple", Repo: "diff-lsp"}}, + }, + { + name: "malformed entries are skipped, valid ones kept", + entries: []string{"not-a-repo", "C-Hipple/diff-lsp", "too/many/parts"}, + want: []jira.RepoRef{{Owner: "C-Hipple", Repo: "diff-lsp"}}, + }, + { + name: "duplicates collapse so a repo is only fetched once", + entries: []string{"C-Hipple/diff-lsp", "C-Hipple/diff-lsp"}, + want: []jira.RepoRef{{Owner: "C-Hipple", Repo: "diff-lsp"}}, + }, + { + name: "no valid entries", + entries: []string{"nope"}, + want: []jira.RepoRef{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := resolveRepoRefs(tt.entries) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("resolveRepoRefs(%v) = %v, want %v", tt.entries, got, tt.want) + } + }) + } +} + +// A project spanning several repos should be configurable as one workflow, so +// the whole Repos list has to survive the build. +func TestBuildProjectListWorkflowKeepsAllRepos(t *testing.T) { + globalRepos := []string{"C-Hipple/global-repo"} + + tests := []struct { + name string + raw config.RawWorkflow + wantRepos []string + }{ + { + name: "explicit multi-repo list", + raw: config.RawWorkflow{ + WorkflowType: "ProjectListWorkflow", + Name: "Project - Multi", + SectionTitle: "Multi Repo Project", + JiraEpic: "BOARD-123", + Repos: []string{"C-Hipple/code-review-server", "C-Hipple/diff-lsp"}, + }, + wantRepos: []string{"C-Hipple/code-review-server", "C-Hipple/diff-lsp"}, + }, + { + name: "falls back to the root-level Repos list", + raw: config.RawWorkflow{ + WorkflowType: "ProjectListWorkflow", + Name: "Project - Global", + SectionTitle: "Global Repo Project", + JiraEpic: "BOARD-123", + }, + wantRepos: globalRepos, + }, + { + name: "legacy singular Owner/Repo still works", + raw: config.RawWorkflow{ + WorkflowType: "ProjectListWorkflow", + Name: "Project - Legacy", + SectionTitle: "Legacy Project", + JiraEpic: "BOARD-123", + Owner: "C-Hipple", + Repo: "diff-lsp", + }, + wantRepos: []string{"C-Hipple/diff-lsp"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repos := append([]string{}, globalRepos...) + built, err := BuildProjectListWorkflow(&tt.raw, &repos, "https://example.atlassian.net") + if err != nil { + t.Fatalf("BuildProjectListWorkflow returned an error: %v", err) + } + wf, ok := built.(ProjectListWorkflow) + if !ok { + t.Fatalf("expected a ProjectListWorkflow, got %T", built) + } + if !reflect.DeepEqual(wf.Repos, tt.wantRepos) { + t.Errorf("Repos = %v, want %v", wf.Repos, tt.wantRepos) + } + if wf.JiraEpic != tt.raw.JiraEpic { + t.Errorf("JiraEpic = %q, want %q", wf.JiraEpic, tt.raw.JiraEpic) + } + }) + } +} + +// The manager collects PRs up front for workflows that declare requirements; +// ProjectListWorkflow resolves its PRs via Jira at run time instead, for every +// repo it spans. +func TestProjectListWorkflowDeclaresNoRequirements(t *testing.T) { + wf := ProjectListWorkflow{ + Repos: []string{"C-Hipple/code-review-server", "C-Hipple/diff-lsp"}, + JiraEpic: "BOARD-123", + } + if reqs := wf.GetPRRequirements(); reqs != nil { + t.Errorf("GetPRRequirements() = %v, want nil", reqs) + } +} diff --git a/workflows/validate.go b/workflows/validate.go index 0677569..0770987 100644 --- a/workflows/validate.go +++ b/workflows/validate.go @@ -48,7 +48,7 @@ var workflowTypes = []WorkflowTypeInfo{ }, { Name: "ProjectListWorkflow", - Description: "Sync the pull requests linked to the children of a Jira epic. Requires JiraDomain plus the JIRA_API_TOKEN and JIRA_API_EMAIL environment variables.", + Description: "Sync the pull requests linked to the children of a Jira epic. Repos takes a list, so one workflow can cover a project spanning several repositories. Requires JiraDomain plus the JIRA_API_TOKEN and JIRA_API_EMAIL environment variables.", RequiredFields: []string{"JiraEpic"}, OptionalFields: []string{"Repos", "Filters", "IncludeDiff", "DesktopNotifications"}, }, diff --git a/workflows/workflows.go b/workflows/workflows.go index d382640..3054d3e 100644 --- a/workflows/workflows.go +++ b/workflows/workflows.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "log/slog" + "strings" "sync" "github.com/google/go-github/v74/github" @@ -177,43 +178,33 @@ func (w ProjectListWorkflow) Run(prs []*github.PullRequest, c chan FileChanges, return RunResult{}, errors.New("ProjectList requires at least one repo") } - // Resolve each configured "owner/repo" to its short name so the JIRA filter - // (which only sees the repo short name in PR URLs) can match and we can map - // results back to the correct owner when fetching from GitHub. - type repoRef struct{ owner, repo string } - shortToRef := make(map[string]repoRef, len(w.Repos)) - shortRepos := make([]string, 0, len(w.Repos)) - for _, entry := range w.Repos { - owner, repo, err := git_tools.ParseRepoName(entry) - if err != nil { - slog.Error("Skipping invalid repo entry", "entry", entry, "error", err) - continue - } - shortToRef[repo] = repoRef{owner: owner, repo: repo} - shortRepos = append(shortRepos, repo) - } - if len(shortRepos) == 0 { + // Resolve each configured "owner/repo" entry so the Jira lookup can match the + // PR links it finds and we can fetch each PR from the right repo. + refs := resolveRepoRefs(w.Repos) + if len(refs) == 0 { return RunResult{}, errors.New("ProjectList has no valid repos") } - prsByRepo := jira.GetProjectPRKeys(w.JiraDomain, w.JiraEpic, shortRepos) + prsByRepo := jira.GetProjectPRKeys(w.JiraDomain, w.JiraEpic, refs) + // Walk the configured order rather than the map so each cycle processes the + // repos the same way. var allPRs []*github.PullRequest - for short, nums := range prsByRepo { + for _, ref := range refs { + nums := prsByRepo[ref] if len(nums) == 0 { continue } - ref := shortToRef[short] - repoPRs, err := git_tools.GetSpecificPRs(client, ref.owner, ref.repo, nums) + repoPRs, err := git_tools.GetSpecificPRs(client, ref.Owner, ref.Repo, nums) if err != nil { - slog.Error("Error getting specific PRs", "owner", ref.owner, "repo", ref.repo, "error", err) + slog.Error("Error getting specific PRs", "owner", ref.Owner, "repo", ref.Repo, "error", err) continue } // Because GetPRRequirements returns nil, the manager's prefetch pass skips // these PRs entirely — leaving the diff (and other aux) caches unpopulated // when GetPRDetails is later called from the web UI. Pre-fetch all aux data // here so it lands in the DB caches and the global AuxDataStore. - prefetchAuxDataForPRs(client, ref.owner, ref.repo, repoPRs) + prefetchAuxDataForPRs(client, ref.Owner, ref.Repo, repoPRs) allPRs = append(allPRs, repoPRs...) } @@ -227,6 +218,28 @@ func (w ProjectListWorkflow) Run(prs []*github.PullRequest, c chan FileChanges, return result, nil } +// resolveRepoRefs turns configured "owner/repo" entries into repo refs, keeping +// the configured order, dropping malformed entries and collapsing duplicates so +// a repo listed twice is only fetched once. +func resolveRepoRefs(entries []string) []jira.RepoRef { + refs := make([]jira.RepoRef, 0, len(entries)) + seen := make(map[jira.RepoRef]struct{}, len(entries)) + for _, entry := range entries { + owner, repo, err := git_tools.ParseRepoName(strings.TrimSpace(entry)) + if err != nil { + slog.Error("Skipping invalid repo entry", "entry", entry, "error", err) + continue + } + ref := jira.RepoRef{Owner: owner, Repo: repo} + if _, dup := seen[ref]; dup { + continue + } + seen[ref] = struct{}{} + refs = append(refs, ref) + } + return refs +} + // prefetchAuxDataForPRs fetches all auxiliary data (diff, comments, reviews, // commits, CI status) for the given PRs and persists it to the DB caches and // the current global AuxDataStore. Used by workflows whose PR set isn't known