Skip to content

Commit

Permalink
ci: fixes bot workflow and comment update (gnolang#3229)
Browse files Browse the repository at this point in the history
This PR should (normally) fix the issues with the bot on this repo. 
In addition to the fixes, I also replaced the old config with a config
that has much simpler rules while we decide what to add later on, once
we've verified that everything is working properly.

Here is the current config, If nothing seems off to you, we can merge it
as it is then improve it incrementaly.

```go
auto := []automaticCheck{
  {
    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(),
    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: "The pull request description provides enough details",
    ifC:         c.Not(c.AuthorInTeam(gh, "core-contributors")),
    teams:       Teams{"core-contributors"},
  },
  {
    description: "Determine if infra needs to be updated before merging",
    ifC: c.And(
      c.BaseBranch("master"),
      c.Or(
        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: Teams{"devops"},
  },
}
```


<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
</details>
  • Loading branch information
aeddi authored and r3v4s committed Dec 10, 2024
1 parent 4fcb1ba commit f24ebea
Show file tree
Hide file tree
Showing 15 changed files with 126 additions and 81 deletions.
12 changes: 7 additions & 5 deletions .github/workflows/bot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
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
# that a same PR cannot be processed concurrently by mutliple runners
Expand All @@ -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
9 changes: 3 additions & 6 deletions contribs/github-bot/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions contribs/github-bot/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
71 changes: 29 additions & 42 deletions contribs/github-bot/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -17,73 +19,58 @@ 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
// in which the GitHub client is injected.
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"},
},
}

Expand Down
2 changes: 1 addition & 1 deletion contribs/github-bot/internal/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions contribs/github-bot/internal/conditions/branch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
21 changes: 21 additions & 0 deletions contribs/github-bot/internal/conditions/draft.go
Original file line number Diff line number Diff line change
@@ -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{}
}
34 changes: 34 additions & 0 deletions contribs/github-bot/internal/conditions/draft_test.go
Original file line number Diff line number Diff line change
@@ -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))
})
}
}
4 changes: 2 additions & 2 deletions contribs/github-bot/internal/conditions/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions contribs/github-bot/internal/conditions/label_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion contribs/github-bot/internal/params/prlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions contribs/github-bot/internal/requirements/assignee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
})
}
}
5 changes: 5 additions & 0 deletions contribs/github-bot/internal/requirements/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 7 additions & 14 deletions contribs/github-bot/internal/requirements/label_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,10 @@ 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()
Expand Down Expand Up @@ -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")
})
}
}
8 changes: 7 additions & 1 deletion contribs/github-bot/matrix.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit f24ebea

Please sign in to comment.