From 24d2864e05b03e1260cfcc9ec0de4545bd5a3ebc Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 11:48:26 +0900 Subject: [PATCH 1/9] test: fix assignee and label tests --- .../internal/requirements/assignee_test.go | 6 ++--- .../internal/requirements/label_test.go | 25 +++++++------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/contribs/github-bot/internal/requirements/assignee_test.go b/contribs/github-bot/internal/requirements/assignee_test.go index df6ffdf0cd3..d72e8ad2a19 100644 --- a/contribs/github-bot/internal/requirements/assignee_test.go +++ b/contribs/github-bot/internal/requirements/assignee_test.go @@ -64,9 +64,9 @@ func TestAssignee(t *testing.T) { details := treeprint.New() requirement := Assignee(gh, testCase.user) - assert.False(t, !requirement.IsSatisfied(pr, details) && !testCase.dryRun, "requirement should have a satisfied status: true") - assert.False(t, !utils.TestLastNodeStatus(t, true, details) && !testCase.dryRun, "requirement details should have a status: true") - assert.False(t, !testCase.exists && !requested && !testCase.dryRun, "requirement should have requested to create item") + assert.True(t, requirement.IsSatisfied(pr, details) || testCase.dryRun, "requirement should have a satisfied status: true") + assert.True(t, utils.TestLastNodeStatus(t, true, details) || testCase.dryRun, "requirement details should have a status: true") + assert.True(t, testCase.exists || requested || testCase.dryRun, "requirement should have requested to create item") }) } } diff --git a/contribs/github-bot/internal/requirements/label_test.go b/contribs/github-bot/internal/requirements/label_test.go index 6fbe8ff7f25..1ba2109848a 100644 --- a/contribs/github-bot/internal/requirements/label_test.go +++ b/contribs/github-bot/internal/requirements/label_test.go @@ -16,7 +16,7 @@ import ( ) func TestLabel(t *testing.T) { - t.Parallel() + // t.Parallel() labels := []*github.Label{ {Name: github.String("notTheRightOne")}, @@ -32,20 +32,13 @@ func TestLabel(t *testing.T) { exists bool }{ {"empty label list", "label", []*github.Label{}, false, false}, - {"empty label list with dry-run", "label", []*github.Label{}, true, false}, - {"label list contains exact match", "label", labels, false, true}, - {"label list contains prefix match", "^lab", labels, false, true}, - {"label list contains prefix doesn't match", "lab$", labels, false, false}, - {"label list contains prefix doesn't match with dry-run", "lab$", labels, true, false}, - {"label list contains suffix match", "bel$", labels, false, true}, - {"label list contains suffix match with dry-run", "bel$", labels, true, true}, - {"label list contains suffix doesn't match", "^bel", labels, false, false}, - {"label list contains suffix doesn't match with dry-run", "^bel", labels, true, false}, - {"label list doesn't contains match", "baleb", labels, false, false}, - {"label list doesn't contains match with dry-run", "baleb", labels, true, false}, + {"empty label list with dry-run", "user", []*github.Label{}, true, false}, + {"label list contains label", "label", labels, false, true}, + {"label list doesn't contain label", "label2", labels, false, false}, + {"label list doesn't contain label with dry-run", "label", labels, true, false}, } { t.Run(testCase.name, func(t *testing.T) { - t.Parallel() + // t.Parallel() requested := false mockedHTTPClient := mock.NewMockedHTTPClient( @@ -71,9 +64,9 @@ func TestLabel(t *testing.T) { details := treeprint.New() requirement := Label(gh, testCase.pattern) - assert.False(t, !requirement.IsSatisfied(pr, details) && !testCase.dryRun, "requirement should have a satisfied status: true") - assert.False(t, !utils.TestLastNodeStatus(t, true, details) && !testCase.dryRun, "requirement details should have a status: true") - assert.False(t, !testCase.exists && !requested && !testCase.dryRun, "requirement should have requested to create item") + assert.True(t, requirement.IsSatisfied(pr, details) || testCase.dryRun, "requirement should have a satisfied status: true") + assert.True(t, utils.TestLastNodeStatus(t, true, details) || testCase.dryRun, "requirement details should have a status: true") + assert.True(t, testCase.exists || requested || testCase.dryRun, "requirement should have requested to create item") }) } } From 2788c0567c9347d35031398228afe2872d07268f Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 11:49:07 +0900 Subject: [PATCH 2/9] feat: add draft/ready condition --- .../github-bot/internal/conditions/draft.go | 21 ++++++++++++ .../internal/conditions/draft_test.go | 34 +++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 contribs/github-bot/internal/conditions/draft.go create mode 100644 contribs/github-bot/internal/conditions/draft_test.go diff --git a/contribs/github-bot/internal/conditions/draft.go b/contribs/github-bot/internal/conditions/draft.go new file mode 100644 index 00000000000..2c263f2ae75 --- /dev/null +++ b/contribs/github-bot/internal/conditions/draft.go @@ -0,0 +1,21 @@ +package conditions + +import ( + "github.com/gnolang/gno/contribs/github-bot/internal/utils" + + "github.com/google/go-github/v64/github" + "github.com/xlab/treeprint" +) + +// Draft Condition. +type draft struct{} + +var _ Condition = &baseBranch{} + +func (*draft) IsMet(pr *github.PullRequest, details treeprint.Tree) bool { + return utils.AddStatusNode(pr.GetDraft(), "This pull request is a draft", details) +} + +func Draft() Condition { + return &draft{} +} diff --git a/contribs/github-bot/internal/conditions/draft_test.go b/contribs/github-bot/internal/conditions/draft_test.go new file mode 100644 index 00000000000..a31b4eaca4c --- /dev/null +++ b/contribs/github-bot/internal/conditions/draft_test.go @@ -0,0 +1,34 @@ +package conditions + +import ( + "fmt" + "testing" + + "github.com/gnolang/gno/contribs/github-bot/internal/utils" + "github.com/google/go-github/v64/github" + "github.com/stretchr/testify/assert" + "github.com/xlab/treeprint" +) + +func TestDraft(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + isMet bool + }{ + {"draft is true", true}, + {"draft is false", false}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + pr := &github.PullRequest{Draft: &testCase.isMet} + details := treeprint.New() + condition := Draft() + + assert.Equal(t, condition.IsMet(pr, details), testCase.isMet, fmt.Sprintf("condition should have a met status: %t", testCase.isMet)) + assert.True(t, utils.TestLastNodeStatus(t, testCase.isMet, details), fmt.Sprintf("condition details should have a status: %t", testCase.isMet)) + }) + } +} From 0f453d8ecbe7a7c5e6d3626cb74de766a516c0b9 Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 12:09:20 +0900 Subject: [PATCH 3/9] fix: matrix subcmd output and workflow --- .github/workflows/bot.yml | 10 +-- contribs/github-bot/config.go | 71 ++++++++----------- contribs/github-bot/internal/params/prlist.go | 2 +- contribs/github-bot/matrix.go | 8 ++- 4 files changed, 43 insertions(+), 48 deletions(-) diff --git a/.github/workflows/bot.yml b/.github/workflows/bot.yml index 5beac27c07e..79ea430a9b5 100644 --- a/.github/workflows/bot.yml +++ b/.github/workflows/bot.yml @@ -44,16 +44,18 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + - name: Install Go uses: actions/setup-go@v5 with: - go-version-file: go.mod + go-version-file: contribs/github-bot/go.mod + - name: Generate matrix from event id: pr-numbers working-directory: contribs/github-bot env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: go run . matrix >> "$GITHUB_OUTPUT" + run: echo "pr-numbers=$(go run . matrix)" >> "$GITHUB_OUTPUT" # This job processes each pull request in the matrix individually while ensuring # that a same PR cannot be processed concurrently by mutliple runners @@ -76,10 +78,10 @@ jobs: - name: Install Go uses: actions/setup-go@v5 with: - go-version-file: go.mod + go-version-file: contribs/github-bot/go.mod - name: Run GitHub Bot working-directory: contribs/github-bot env: GITHUB_TOKEN: ${{ secrets.GH_BOT_PAT }} - run: go run . -pr-numbers '${{ matrix.pr-number }}' -verbose + run: go run . check -pr-numbers '${{ matrix.pr-number }}' -verbose diff --git a/contribs/github-bot/config.go b/contribs/github-bot/config.go index 4504844e289..4a28565ef7f 100644 --- a/contribs/github-bot/config.go +++ b/contribs/github-bot/config.go @@ -6,6 +6,8 @@ import ( r "github.com/gnolang/gno/contribs/github-bot/internal/requirements" ) +type Teams []string + // Automatic check that will be performed by the bot. type automaticCheck struct { description string @@ -17,7 +19,7 @@ type automaticCheck struct { type manualCheck struct { description string ifC c.Condition // If the condition is met, a checkbox will be displayed on bot comment. - teams []string // Members of these teams can check the checkbox to make the check pass. + teams Teams // Members of these teams can check the checkbox to make the check pass. } // This function returns the configuration of the bot consisting of automatic and manual checks @@ -25,65 +27,50 @@ type manualCheck struct { func config(gh *client.GitHub) ([]automaticCheck, []manualCheck) { auto := []automaticCheck{ { - description: "Changes to 'tm2' folder should be reviewed/authored by at least one member of both EU and US teams", - ifC: c.And( - c.FileChanged(gh, "tm2"), - c.BaseBranch("master"), - ), - thenR: r.And( - r.Or( - r.ReviewByTeamMembers(gh, "eu", 1), - r.AuthorInTeam(gh, "eu"), - ), - r.Or( - r.ReviewByTeamMembers(gh, "us", 1), - r.AuthorInTeam(gh, "us"), - ), - ), - }, - { - description: "A maintainer must be able to edit this pull request", + description: "Maintainers must be able to edit this pull request", ifC: c.Always(), thenR: r.MaintainerCanModify(), }, { description: "The pull request head branch must be up-to-date with its base", - ifC: c.Always(), // Or only if c.BaseBranch("main") ? + ifC: c.Always(), thenR: r.UpToDateWith(gh, r.PR_BASE), }, + { + description: "Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff", + ifC: c.FileChanged(gh, "^docs/"), + thenR: r.Or( + r.And( + r.AuthorInTeam(gh, "devrels"), + r.ReviewByTeamMembers(gh, "tech-staff", 1), + ), + r.And( + r.AuthorInTeam(gh, "tech-staff"), + r.ReviewByTeamMembers(gh, "devrels", 1), + ), + ), + }, } manual := []manualCheck{ { - description: "Determine if infra needs to be updated", - ifC: c.And( - c.BaseBranch("master"), - c.Or( - c.FileChanged(gh, "misc/deployments"), - c.FileChanged(gh, `misc/docker-\.*`), - c.FileChanged(gh, "tm2/pkg/p2p"), - ), - ), - teams: []string{"tech-staff"}, + description: "The pull request description provides enough details", + ifC: c.Not(c.AuthorInTeam(gh, "core-contributors")), + teams: Teams{"core-contributors"}, }, { - description: "Ensure the code style is satisfactory", + description: "Determine if infra needs to be updated before merging", ifC: c.And( c.BaseBranch("master"), c.Or( - c.FileChanged(gh, `.*\.go`), - c.FileChanged(gh, `.*\.js`), + c.FileChanged(gh, `Dockerfile`), + c.FileChanged(gh, `^misc/deployments`), + c.FileChanged(gh, `^misc/docker-`), + c.FileChanged(gh, `^.github/workflows/releaser.*\.yml$`), + c.FileChanged(gh, `^.github/workflows/portal-loop\.yml$`), ), ), - teams: []string{"tech-staff"}, - }, - { - description: "Ensure the documentation is accurate and relevant", - ifC: c.FileChanged(gh, `.*\.md`), - teams: []string{ - "tech-staff", - "devrels", - }, + teams: Teams{"devops"}, }, } diff --git a/contribs/github-bot/internal/params/prlist.go b/contribs/github-bot/internal/params/prlist.go index 51aed8dc457..ace7bcbe3b6 100644 --- a/contribs/github-bot/internal/params/prlist.go +++ b/contribs/github-bot/internal/params/prlist.go @@ -23,7 +23,7 @@ func (p PRList) MarshalText() (text []byte, err error) { prNumsStr[i] = strconv.Itoa(prNum) } - return []byte(strings.Join(prNumsStr, ",")), nil + return []byte(strings.Join(prNumsStr, ", ")), nil } // UnmarshalText implements encoding.TextUnmarshaler. diff --git a/contribs/github-bot/matrix.go b/contribs/github-bot/matrix.go index 2442a6d94d6..56d6667589a 100644 --- a/contribs/github-bot/matrix.go +++ b/contribs/github-bot/matrix.go @@ -48,7 +48,13 @@ func execMatrix() error { return err } - fmt.Println(prList) + // Print PR list for GitHub Actions matrix definition + bytes, err := prList.MarshalText() + if err != nil { + return fmt.Errorf("unable to marshal PR list: %w", err) + } + fmt.Printf("[%s]", string(bytes)) + return nil } From 657976bb4d3ecca00e4f6c4a6a69d9eefaff8997 Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 17:17:29 +0900 Subject: [PATCH 4/9] docs: update README --- contribs/github-bot/README.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/contribs/github-bot/README.md b/contribs/github-bot/README.md index e3cc12fe01a..78c9c3c01b8 100644 --- a/contribs/github-bot/README.md +++ b/contribs/github-bot/README.md @@ -27,14 +27,11 @@ For the bot to make requests to the GitHub API, it needs a Personal Access Token ## Usage ```bash -> go install github.com/gnolang/gno/contribs/github-bot@latest -// (go: downloading ...) - -> github-bot --help +> github-bot check --help USAGE - github-bot [flags] + github-bot check [flags] -This tool checks if the requirements for a PR to be merged are satisfied (defined in config.go) and displays PR status checks accordingly. +This tool checks if the requirements for a pull request to be merged are satisfied (defined in config.go) and displays PR status checks accordingly. A valid GitHub Token must be provided by setting the GITHUB_TOKEN environment variable. FLAGS From 300b7a9e5f977b47ac08a344487ac06bcb521a21 Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 19:22:04 +0900 Subject: [PATCH 5/9] fix: bot comment update --- contribs/github-bot/comment.go | 4 ++-- contribs/github-bot/internal/client/client.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contribs/github-bot/comment.go b/contribs/github-bot/comment.go index 8bf4a158745..f6605ea8554 100644 --- a/contribs/github-bot/comment.go +++ b/contribs/github-bot/comment.go @@ -175,9 +175,9 @@ func handleCommentUpdate(gh *client.GitHub, actionCtx *githubactions.GitHubConte // If teams specified in rule, check if actor is a member of one of them. if len(teams) > 0 { - if gh.IsUserInTeams(actionCtx.Actor, teams) { + if !gh.IsUserInTeams(actionCtx.Actor, teams) { // If user not allowed if !gh.DryRun { - gh.SetBotComment(previous, int(prNum)) + gh.SetBotComment(previous, int(prNum)) // Restore previous state } return errors.New("checkbox edited by a user not allowed to") } diff --git a/contribs/github-bot/internal/client/client.go b/contribs/github-bot/internal/client/client.go index 229c3e90631..474146ad3da 100644 --- a/contribs/github-bot/internal/client/client.go +++ b/contribs/github-bot/internal/client/client.go @@ -80,7 +80,7 @@ func (gh *GitHub) GetBotComment(prNum int) (*github.IssueComment, error) { opts.Page = response.NextPage } - return nil, errors.New("bot comment not found") + return nil, ErrBotCommentNotFound } // SetBotComment creates a bot's comment on the provided PR number From 09f0cf7bf6e2d91d09fc1b5a5e0ae3fc1502aa2a Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 19:23:36 +0900 Subject: [PATCH 6/9] fix: branch comparison across forks --- contribs/github-bot/internal/requirements/branch.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contribs/github-bot/internal/requirements/branch.go b/contribs/github-bot/internal/requirements/branch.go index 65d00d06ae8..b686a093015 100644 --- a/contribs/github-bot/internal/requirements/branch.go +++ b/contribs/github-bot/internal/requirements/branch.go @@ -27,7 +27,12 @@ func (u *upToDateWith) IsSatisfied(pr *github.PullRequest, details treeprint.Tre if u.base == PR_BASE { base = pr.GetBase().GetRef() } + head := pr.GetHead().GetRef() + // If pull request is open from a fork, prepend head ref with fork owner login + if pr.GetHead().GetRepo().GetFullName() != pr.GetBase().GetRepo().GetFullName() { + head = fmt.Sprintf("%s:%s", pr.GetHead().GetRepo().GetOwner().GetLogin(), pr.GetHead().GetRef()) + } cmp, _, err := u.gh.Client.Repositories.CompareCommits(u.gh.Ctx, u.gh.Owner, u.gh.Repo, base, head, nil) if err != nil { From 3f3d4269f72ca4305a784df1d39535110e1e1db5 Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 19:24:03 +0900 Subject: [PATCH 7/9] test: fix regex based unit tests --- contribs/github-bot/internal/conditions/branch_test.go | 4 ++-- contribs/github-bot/internal/conditions/file_test.go | 4 ++-- contribs/github-bot/internal/conditions/label_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contribs/github-bot/internal/conditions/branch_test.go b/contribs/github-bot/internal/conditions/branch_test.go index 3e53ef2db1c..81ed96f8314 100644 --- a/contribs/github-bot/internal/conditions/branch_test.go +++ b/contribs/github-bot/internal/conditions/branch_test.go @@ -22,9 +22,9 @@ func TestHeadBaseBranch(t *testing.T) { }{ {"perfectly match", "base", "base", true}, {"prefix match", "^dev/", "dev/test-bot", true}, - {"prefix doesn't match", "dev/$", "dev/test-bot", false}, + {"prefix doesn't match", "^/test-bot", "dev/test-bot", false}, {"suffix match", "/test-bot$", "dev/test-bot", true}, - {"suffix doesn't match", "^/test-bot", "dev/test-bot", false}, + {"suffix doesn't match", "dev/$", "dev/test-bot", false}, {"doesn't match", "base", "notatall", false}, } { t.Run(testCase.name, func(t *testing.T) { diff --git a/contribs/github-bot/internal/conditions/file_test.go b/contribs/github-bot/internal/conditions/file_test.go index 3fd7a33fa4a..8571ffea7d0 100644 --- a/contribs/github-bot/internal/conditions/file_test.go +++ b/contribs/github-bot/internal/conditions/file_test.go @@ -33,9 +33,9 @@ func TestFileChanged(t *testing.T) { {"empty file list", "foo", []*github.CommitFile{}, false}, {"file list contains exact match", "foo", filenames, true}, {"file list contains prefix match", "^fo", filenames, true}, - {"file list contains prefix doesn't match", "fo$", filenames, false}, + {"file list contains prefix doesn't match", "^oo", filenames, false}, {"file list contains suffix match", "oo$", filenames, true}, - {"file list contains suffix doesn't match", "^oo", filenames, false}, + {"file list contains suffix doesn't match", "fo$", filenames, false}, {"file list doesn't contains match", "foobar", filenames, false}, } { t.Run(testCase.name, func(t *testing.T) { diff --git a/contribs/github-bot/internal/conditions/label_test.go b/contribs/github-bot/internal/conditions/label_test.go index ea895b28ad1..00a3a8e3457 100644 --- a/contribs/github-bot/internal/conditions/label_test.go +++ b/contribs/github-bot/internal/conditions/label_test.go @@ -29,9 +29,9 @@ func TestLabel(t *testing.T) { {"empty label list", "label", []*github.Label{}, false}, {"label list contains exact match", "label", labels, true}, {"label list contains prefix match", "^lab", labels, true}, - {"label list contains prefix doesn't match", "lab$", labels, false}, + {"label list contains prefix doesn't match", "^bel", labels, false}, {"label list contains suffix match", "bel$", labels, true}, - {"label list contains suffix doesn't match", "^bel", labels, false}, + {"label list contains suffix doesn't match", "lab$", labels, false}, {"label list doesn't contains match", "baleb", labels, false}, } { t.Run(testCase.name, func(t *testing.T) { From d9cc924bfda20edd85eb8b07ce47a416a3c3c240 Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 20:28:45 +0900 Subject: [PATCH 8/9] ci: fix env variable name --- .github/workflows/bot.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bot.yml b/.github/workflows/bot.yml index 79ea430a9b5..21950459ae8 100644 --- a/.github/workflows/bot.yml +++ b/.github/workflows/bot.yml @@ -54,7 +54,7 @@ jobs: id: pr-numbers working-directory: contribs/github-bot env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: echo "pr-numbers=$(go run . matrix)" >> "$GITHUB_OUTPUT" # This job processes each pull request in the matrix individually while ensuring From 2ae172e45d7b055f9646d714c8ec924f40e446fa Mon Sep 17 00:00:00 2001 From: aeddi Date: Thu, 28 Nov 2024 23:20:49 +0900 Subject: [PATCH 9/9] test: remove leftovers --- contribs/github-bot/internal/requirements/label_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contribs/github-bot/internal/requirements/label_test.go b/contribs/github-bot/internal/requirements/label_test.go index 1ba2109848a..7e991b55756 100644 --- a/contribs/github-bot/internal/requirements/label_test.go +++ b/contribs/github-bot/internal/requirements/label_test.go @@ -16,7 +16,7 @@ import ( ) func TestLabel(t *testing.T) { - // t.Parallel() + t.Parallel() labels := []*github.Label{ {Name: github.String("notTheRightOne")}, @@ -38,7 +38,7 @@ func TestLabel(t *testing.T) { {"label list doesn't contain label with dry-run", "label", labels, true, false}, } { t.Run(testCase.name, func(t *testing.T) { - // t.Parallel() + t.Parallel() requested := false mockedHTTPClient := mock.NewMockedHTTPClient(