internal/task: move loadMilestoneIssues method into GitHubClient
TestMilestones is a destructive test that operates on a real GitHub
repo. This was useful during the initial development of MilestoneTasks,
but now it's preferable to have a small and quick-to-run test to go
alongside small behavior adjustments we might want to make.
It was tempting to add a test-only hook like:
func (m *MilestoneTasks) loadMilestoneIssues(...) {
+ if m.testHookLoadMilestoneIssues != nil {
+ // easily return milestone issues for a test case
+ }
// real implementation
But we already have GitHubClientInterface that's meant to help.
It's possible to mock its Query method, but doing that is very
verbose and fragile. Instead, factor in the loadMilestoneIssues
method to be a part of GitHubClientInterface since it fits well.
While here, also s/load/fetch/ in its name for consistency.
As a result, we now have TestCheckBlockers. The next CL in the stack
changes how beta 2/3/etc. are handled to match our previous behavior.
The one after that adds handling for the new okay-after-rc1 label.
Change-Id: I8fb8df6eedd668c08ea954b3e29176a654801a36
Reviewed-on: https://go-review.googlesource.com/c/build/+/453980
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go
index 752c2e5..381ca44 100644
--- a/internal/relui/buildrelease_test.go
+++ b/internal/relui/buildrelease_test.go
@@ -196,7 +196,7 @@
},
}
milestoneTasks := &task.MilestoneTasks{
- Client: &fakeGitHub{},
+ Client: fakeGitHub{},
RepoOwner: "golang",
RepoName: "go",
ApproveAction: func(ctx *workflow.TaskContext) error {
@@ -639,19 +639,19 @@
type fakeGitHub struct{}
-func (g *fakeGitHub) FetchMilestone(ctx context.Context, owner, repo, name string, create bool) (int, error) {
+func (fakeGitHub) FetchMilestone(_ context.Context, owner, repo, name string, create bool) (int, error) {
return 0, nil
}
-func (g *fakeGitHub) Query(ctx context.Context, q interface{}, variables map[string]interface{}) error {
- return nil
+func (fakeGitHub) FetchMilestoneIssues(_ context.Context, owner, repo string, milestoneID int) (map[int]map[string]bool, error) {
+ return nil, nil
}
-func (g *fakeGitHub) EditIssue(ctx context.Context, owner string, repo string, number int, issue *github.IssueRequest) (*github.Issue, *github.Response, error) {
+func (fakeGitHub) EditIssue(_ context.Context, owner string, repo string, number int, issue *github.IssueRequest) (*github.Issue, *github.Response, error) {
return nil, nil, nil
}
-func (g *fakeGitHub) EditMilestone(ctx context.Context, owner string, repo string, number int, milestone *github.Milestone) (*github.Milestone, *github.Response, error) {
+func (fakeGitHub) EditMilestone(_ context.Context, owner string, repo string, number int, milestone *github.Milestone) (*github.Milestone, *github.Response, error) {
return nil, nil, nil
}
diff --git a/internal/task/milestones.go b/internal/task/milestones.go
index 8f259a9..08b3e77 100644
--- a/internal/task/milestones.go
+++ b/internal/task/milestones.go
@@ -90,7 +90,7 @@
// label to suppress them.
return nil
}
- issues, err := m.loadMilestoneIssues(ctx, milestones.Current, kind)
+ issues, err := m.Client.FetchMilestoneIssues(ctx, m.RepoOwner, m.RepoName, milestones.Current)
if err != nil {
return err
}
@@ -113,61 +113,6 @@
return m.ApproveAction(ctx)
}
-// loadMilestoneIssues returns all the open issues in the specified milestone
-// and their labels.
-func (m *MilestoneTasks) loadMilestoneIssues(ctx *wf.TaskContext, milestoneID int, kind ReleaseKind) (map[int]map[string]bool, error) {
- issues := map[int]map[string]bool{}
- var query struct {
- Repository struct {
- Issues struct {
- PageInfo struct {
- EndCursor githubv4.String
- HasNextPage bool
- }
-
- Nodes []struct {
- Number int
- ID githubv4.ID
- Title string
- Labels struct {
- PageInfo struct {
- HasNextPage bool
- }
- Nodes []struct {
- Name string
- }
- } `graphql:"labels(first:10)"`
- }
- } `graphql:"issues(first:100, after:$afterToken, filterBy:{states:OPEN, milestoneNumber:$milestoneNumber})"`
- } `graphql:"repository(owner: $repoOwner, name: $repoName)"`
- }
- var afterToken *githubv4.String
-more:
- if err := m.Client.Query(ctx, &query, map[string]interface{}{
- "repoOwner": githubv4.String(m.RepoOwner),
- "repoName": githubv4.String(m.RepoName),
- "milestoneNumber": githubv4.String(fmt.Sprint(milestoneID)),
- "afterToken": afterToken,
- }); err != nil {
- return nil, err
- }
- for _, issue := range query.Repository.Issues.Nodes {
- if issue.Labels.PageInfo.HasNextPage {
- return nil, fmt.Errorf("issue %v (#%v) has more than 10 labels", issue.Title, issue.Number)
- }
- labels := map[string]bool{}
- for _, label := range issue.Labels.Nodes {
- labels[label.Name] = true
- }
- issues[issue.Number] = labels
- }
- if query.Repository.Issues.PageInfo.HasNextPage {
- afterToken = &query.Repository.Issues.PageInfo.EndCursor
- goto more
- }
- return issues, nil
-}
-
// PushIssues updates issues to reflect a finished release. For beta1 releases,
// it removes the okay-after-beta1 label. For major and minor releases,
// it moves them to the next milestone and closes the current one.
@@ -177,7 +122,7 @@
return nil
}
- issues, err := m.loadMilestoneIssues(ctx, milestones.Current, KindUnknown)
+ issues, err := m.Client.FetchMilestoneIssues(ctx, m.RepoOwner, m.RepoName, milestones.Current)
if err != nil {
return err
}
@@ -223,8 +168,9 @@
// and the milestone doesn't exist, it will be created.
FetchMilestone(ctx context.Context, owner, repo, name string, create bool) (int, error)
- // See githubv4.Client.Query.
- Query(ctx context.Context, q interface{}, variables map[string]interface{}) error
+ // FetchMilestoneIssues returns all the open issues in the specified milestone
+ // and their labels.
+ FetchMilestoneIssues(ctx context.Context, owner, repo string, milestoneID int) (map[int]map[string]bool, error)
// See github.Client.Issues.Edit.
EditIssue(ctx context.Context, owner string, repo string, number int, issue *github.IssueRequest) (*github.Issue, *github.Response, error)
@@ -238,10 +184,6 @@
V4 *githubv4.Client
}
-func (c *GitHubClient) Query(ctx context.Context, q interface{}, variables map[string]interface{}) error {
- return c.V4.Query(ctx, q, variables)
-}
-
func (c *GitHubClient) FetchMilestone(ctx context.Context, owner, repo, name string, create bool) (int, error) {
n, found, err := findMilestone(ctx, c.V4, owner, repo, name)
if err != nil {
@@ -313,6 +255,59 @@
panic(fmt.Errorf("unhandled case: open: %q closed: %q", open, closed))
}
+func (c *GitHubClient) FetchMilestoneIssues(ctx context.Context, owner, repo string, milestoneID int) (map[int]map[string]bool, error) {
+ issues := map[int]map[string]bool{}
+ var query struct {
+ Repository struct {
+ Issues struct {
+ PageInfo struct {
+ EndCursor githubv4.String
+ HasNextPage bool
+ }
+
+ Nodes []struct {
+ Number int
+ ID githubv4.ID
+ Title string
+ Labels struct {
+ PageInfo struct {
+ HasNextPage bool
+ }
+ Nodes []struct {
+ Name string
+ }
+ } `graphql:"labels(first:10)"`
+ }
+ } `graphql:"issues(first:100, after:$afterToken, filterBy:{states:OPEN, milestoneNumber:$milestoneNumber})"`
+ } `graphql:"repository(owner: $repoOwner, name: $repoName)"`
+ }
+ var afterToken *githubv4.String
+more:
+ if err := c.V4.Query(ctx, &query, map[string]interface{}{
+ "repoOwner": githubv4.String(owner),
+ "repoName": githubv4.String(repo),
+ "milestoneNumber": githubv4.String(fmt.Sprint(milestoneID)),
+ "afterToken": afterToken,
+ }); err != nil {
+ return nil, err
+ }
+ for _, issue := range query.Repository.Issues.Nodes {
+ if issue.Labels.PageInfo.HasNextPage {
+ return nil, fmt.Errorf("issue %v (#%v) has more than 10 labels", issue.Title, issue.Number)
+ }
+ labels := map[string]bool{}
+ for _, label := range issue.Labels.Nodes {
+ labels[label.Name] = true
+ }
+ issues[issue.Number] = labels
+ }
+ if query.Repository.Issues.PageInfo.HasNextPage {
+ afterToken = &query.Repository.Issues.PageInfo.EndCursor
+ goto more
+ }
+ return issues, nil
+}
+
func (c *GitHubClient) EditIssue(ctx context.Context, owner string, repo string, number int, issue *github.IssueRequest) (*github.Issue, *github.Response, error) {
return c.V3.Issues.Edit(ctx, owner, repo, number, issue)
}
diff --git a/internal/task/milestones_test.go b/internal/task/milestones_test.go
index 5952d3b..3cc110d 100644
--- a/internal/task/milestones_test.go
+++ b/internal/task/milestones_test.go
@@ -14,6 +14,86 @@
"golang.org/x/oauth2"
)
+func TestCheckBlockers(t *testing.T) {
+ var errManualApproval = fmt.Errorf("manual approval is required")
+ for _, tc := range [...]struct {
+ name string
+ milestoneIssues map[int]map[string]bool
+ version string
+ kind ReleaseKind
+ want error
+ }{
+ {
+ name: "beta 1 with one hard blocker",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true}},
+ version: "go1.20beta1", kind: KindBeta,
+ want: errManualApproval,
+ },
+ {
+ name: "beta 1 with one blocker marked okay-after-beta1",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true, "okay-after-beta1": true}},
+ version: "go1.20beta1", kind: KindBeta,
+ want: nil, // Want no error.
+ },
+ {
+ name: "beta 2 with one hard blocker and meaningless okay-after-beta1 label",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true, "okay-after-beta1": true}},
+ version: "go1.20beta2", kind: KindBeta,
+ want: nil, // TODO(go.dev/cl/453981): Should be a release-blocker that requires manual approval.
+ },
+ {
+ name: "RC 1 with one hard blocker",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true}},
+ version: "go1.20rc1", kind: KindRC,
+ want: nil, // TODO(go.dev/cl/453982): Should be a release-blocker that requires manual approval.
+ },
+ {
+ name: "RC 1 with one blocker marked okay-after-rc1",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true, "okay-after-rc1": true}},
+ version: "go1.20rc1", kind: KindRC,
+ want: nil, // Want no error.
+ },
+ {
+ name: "RC 2 with one hard blocker and meaningless okay-after-rc1 label",
+ milestoneIssues: map[int]map[string]bool{123: {"release-blocker": true, "okay-after-rc1": true}},
+ version: "go1.20rc2", kind: KindRC,
+ want: nil, // TODO(go.dev/cl/453982): Should be a release-blocker that requires manual approval.
+ },
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ tasks := &MilestoneTasks{
+ Client: fakeGitHub{tc.milestoneIssues},
+ ApproveAction: func(*workflow.TaskContext) error { return errManualApproval },
+ }
+ ctx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t}}
+ got := tasks.CheckBlockers(ctx, ReleaseMilestones{1, 2}, tc.version, tc.kind)
+ if got != tc.want {
+ t.Errorf("got %v, want %v", got, tc.want)
+ }
+ })
+ }
+}
+
+type fakeGitHub struct {
+ milestoneIssues map[int]map[string]bool
+}
+
+func (fakeGitHub) FetchMilestone(_ context.Context, owner, repo, name string, create bool) (int, error) {
+ return 0, nil
+}
+
+func (g fakeGitHub) FetchMilestoneIssues(_ context.Context, owner, repo string, milestoneID int) (map[int]map[string]bool, error) {
+ return g.milestoneIssues, nil
+}
+
+func (fakeGitHub) EditIssue(_ context.Context, owner string, repo string, number int, issue *github.IssueRequest) (*github.Issue, *github.Response, error) {
+ return nil, nil, nil
+}
+
+func (fakeGitHub) EditMilestone(_ context.Context, owner string, repo string, number int, milestone *github.Milestone) (*github.Milestone, *github.Response, error) {
+ return nil, nil, nil
+}
+
var (
flagRun = flag.Bool("run-destructive-milestones-test", false, "Run the milestone test. Requires repository owner and name flags, and GITHUB_TOKEN set in the environment.")
flagOwner = flag.String("milestones-github-owner", "", "Owner of testing repository")
