Skip to content

Commit

Permalink
feat(github-bot): refactor comment + add force skip (gnolang#3311)
Browse files Browse the repository at this point in the history
This PR significantly modifies the github-bot's comment and adds a
button to force the success of its CI check, even it the requirements
provided in the config are not met.

Related to
gnolang#3238 (comment)

**Edit**: I updated [the comment
below](gnolang#3311 (comment))
by running the bot on my laptop if you want to see the result (so the
skip button is not working yet).
  • Loading branch information
aeddi authored Dec 10, 2024
1 parent ed4ebe8 commit 8e7fb50
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 31 deletions.
32 changes: 25 additions & 7 deletions contribs/github-bot/internal/check/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ func processPRList(gh *client.GitHub, prs []*github.PullRequest) error {
go func(pr *github.PullRequest) {
defer wg.Done()
commentContent := CommentContent{}
commentContent.allSatisfied = true
commentContent.AutoAllSatisfied = true
commentContent.ManualAllSatisfied = true

// Iterate over all automatic rules in config.
for _, autoRule := range autoRules {
Expand All @@ -120,7 +121,7 @@ func processPRList(gh *client.GitHub, prs []*github.PullRequest) error {
thenDetails.SetValue(fmt.Sprintf("%s Requirement satisfied", utils.Success))
c.Satisfied = true
} else {
commentContent.allSatisfied = false
commentContent.AutoAllSatisfied = false
}

c.ConditionDetails = ifDetails.String()
Expand Down Expand Up @@ -160,8 +161,14 @@ func processPRList(gh *client.GitHub, prs []*github.PullRequest) error {
},
)

if checkedBy == "" {
commentContent.allSatisfied = false
// If this check is the special one, store its state in the dedicated var.
if manualRule.Description == config.ForceSkipDescription {
if checkedBy != "" {
commentContent.ForceSkip = true
}
} else if checkedBy == "" {
// Or if its a normal check, just verify if it was checked by someone.
commentContent.ManualAllSatisfied = false
}
}

Expand Down Expand Up @@ -224,9 +231,20 @@ func logResults(logger logger.Logger, prNum int, commentContent CommentContent)
}

logger.Infof("Conclusion:")
if commentContent.allSatisfied {
logger.Infof("%s All requirements are satisfied\n", utils.Success)

if commentContent.AutoAllSatisfied {
logger.Infof("%s All automated checks are satisfied", utils.Success)
} else {
logger.Infof("%s Some automated checks are not satisfied", utils.Fail)
}

if commentContent.ManualAllSatisfied {
logger.Infof("%s All manual checks are satisfied\n", utils.Success)
} else {
logger.Infof("%s Not all requirements are satisfied\n", utils.Fail)
logger.Infof("%s Some manual checks are not satisfied\n", utils.Fail)
}

if commentContent.ForceSkip {
logger.Infof("%s Bot checks are force skipped\n", utils.Success)
}
}
30 changes: 17 additions & 13 deletions contribs/github-bot/internal/check/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ var errTriggeredByBot = errors.New("event triggered by bot")
// Compile regex only once.
var (
// Regex for capturing the entire line of a manual check.
manualCheckLine = regexp.MustCompile(`(?m:^-\s\[([ xX])\]\s+(.+?)\s*(\(checked by @(\w+)\))?$)`)
manualCheckLine = regexp.MustCompile(`(?m:^- \[([ xX])\] (.+?)(?: \(checked by @([A-Za-z0-9-]+)\))?$)`)
// Regex for capturing only the checkboxes.
checkboxes = regexp.MustCompile(`(?m:^- \[[ x]\])`)
checkboxes = regexp.MustCompile(`(?m:^- \[[ xX]\])`)
// Regex used to capture markdown links.
markdownLink = regexp.MustCompile(`\[(.*)\]\([^)]*\)`)
)
Expand All @@ -46,9 +46,11 @@ type ManualContent struct {
Teams []string
}
type CommentContent struct {
AutoRules []AutoContent
ManualRules []ManualContent
allSatisfied bool
AutoRules []AutoContent
ManualRules []ManualContent
AutoAllSatisfied bool
ManualAllSatisfied bool
ForceSkip bool
}

type manualCheckDetails struct {
Expand All @@ -64,10 +66,10 @@ func getCommentManualChecks(commentBody string) map[string]manualCheckDetails {
// For each line that matches the "Manual check" regex.
for _, match := range manualCheckLine.FindAllStringSubmatch(commentBody, -1) {
description := match[2]
status := match[1]
status := strings.ToLower(match[1]) // if X captured, convert it to x.
checkedBy := ""
if len(match) > 4 {
checkedBy = strings.ToLower(match[4]) // if X captured, convert it to x.
if len(match) > 3 {
checkedBy = match[3]
}

checks[description] = manualCheckDetails{status: status, checkedBy: checkedBy}
Expand Down Expand Up @@ -261,13 +263,15 @@ func updatePullRequest(gh *client.GitHub, pr *github.PullRequest, content Commen
var (
context = "Merge Requirements"
targetURL = comment.GetHTMLURL()
state = "failure"
description = "Some requirements are not satisfied yet. See bot comment."
state = "success"
description = "All requirements are satisfied."
)

if content.allSatisfied {
state = "success"
description = "All requirements are satisfied."
if content.ForceSkip {
description = "Bot checks are skipped for this PR."
} else if !content.AutoAllSatisfied || !content.ManualAllSatisfied {
state = "failure"
description = "Some requirements are not satisfied yet. See bot comment."
}

// Update or create commit status.
Expand Down
32 changes: 24 additions & 8 deletions contribs/github-bot/internal/check/comment.tmpl
Original file line number Diff line number Diff line change
@@ -1,19 +1,34 @@
I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.
#### 🛠 PR Checks Summary
{{ if and .AutoRules (not .AutoAllSatisfied) }}{{ range .AutoRules }}{{ if not .Satisfied }} 🔴 {{ .Description }}
{{end}}{{end}}{{ else }}All **Automated Checks** passed. ✅{{end}}

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.
##### Manual Checks (for Reviewers):
{{ if .ManualRules }}{{ range .ManualRules }}- [{{ if .CheckedBy }}x{{ else }} {{ end }}] {{ .Description }}{{ if .CheckedBy }} (checked by @{{ .CheckedBy }}){{ end }}
{{ end }}{{ else }}*No manual checks match this pull request.*{{ end }}

These requirements are defined in this [configuration file](https://github.com/gnolang/gno/tree/master/contribs/github-bot/internal/config/config.go).
<details><summary>Read More</summary>

## Automated Checks
🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

##### ✅ Automated Checks (for Contributors):
{{ if .AutoRules }}{{ range .AutoRules }} {{ if .Satisfied }}🟢{{ else }}🔴{{ end }} {{ .Description }}
{{ end }}{{ else }}*No automated checks match this pull request.*{{ end }}

## Manual Checks
##### ☑️ Contributor Actions:
1. Fix any issues flagged by automated checks.
2. Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include `BREAKING CHANGE` notes.
- Link related issues/PRs, where applicable.

{{ if .ManualRules }}{{ range .ManualRules }}- [{{ if .CheckedBy }}x{{ else }} {{ end }}] {{ .Description }}{{ if .CheckedBy }} (checked by @{{ .CheckedBy }}){{ end }}
{{ end }}{{ else }}*No manual checks match this pull request.*{{ end }}
##### ☑️ Reviewer Actions:
1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.

##### 📚 Resources:
- [Report a bug with the bot](https://github.com/gnolang/gno/issues/3238).
- [View the bot’s configuration file](https://github.com/gnolang/gno/tree/master/contribs/github-bot/internal/config/config.go).

{{ if or .AutoRules .ManualRules }}<details><summary><b>Debug</b></summary><blockquote>
{{ if .AutoRules }}<details><summary><b>Automated Checks</b></summary><blockquote>
Expand Down Expand Up @@ -52,3 +67,4 @@ These requirements are defined in this [configuration file](https://github.com/g
{{ end }}
</blockquote></details>
{{ end }}
</details>
19 changes: 16 additions & 3 deletions contribs/github-bot/internal/check/comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,44 @@ func TestGeneratedComment(t *testing.T) {
{Description: "Test automatic 5", Satisfied: false},
}
manualRules := []ManualContent{
{Description: "Test manual 1", CheckedBy: "user_1"},
{Description: "Test manual 1", CheckedBy: "user-1"},
{Description: "Test manual 2", CheckedBy: ""},
{Description: "Test manual 3", CheckedBy: ""},
{Description: "Test manual 4", CheckedBy: "user_4"},
{Description: "Test manual 5", CheckedBy: "user_5"},
{Description: "Test manual 4", CheckedBy: "user-4"},
{Description: "Test manual 5", CheckedBy: "user-5"},
}

commentText, err := generateComment(content)
assert.Nil(t, err, fmt.Sprintf("error is not nil: %v", err))
assert.True(t, strings.Contains(commentText, "*No automated checks match this pull request.*"), "should contains automated check placeholder")
assert.True(t, strings.Contains(commentText, "*No manual checks match this pull request.*"), "should contains manual check placeholder")
assert.True(t, strings.Contains(commentText, "All **Automated Checks** passed. ✅"), "should contains automated checks passed placeholder")

content.AutoRules = autoRules
content.AutoAllSatisfied = true
commentText, err = generateComment(content)
assert.Nil(t, err, fmt.Sprintf("error is not nil: %v", err))
assert.False(t, strings.Contains(commentText, "*No automated checks match this pull request.*"), "should not contains automated check placeholder")
assert.True(t, strings.Contains(commentText, "*No manual checks match this pull request.*"), "should contains manual check placeholder")
assert.True(t, strings.Contains(commentText, "All **Automated Checks** passed. ✅"), "should contains automated checks passed placeholder")
assert.Equal(t, 2, len(autoCheckSuccessLine.FindAllStringSubmatch(commentText, -1)), "wrong number of succeeded automatic check")
assert.Equal(t, 3, len(autoCheckFailLine.FindAllStringSubmatch(commentText, -1)), "wrong number of failed automatic check")

content.AutoAllSatisfied = false
commentText, err = generateComment(content)
assert.Nil(t, err, fmt.Sprintf("error is not nil: %v", err))
assert.False(t, strings.Contains(commentText, "*No automated checks match this pull request.*"), "should not contains automated check placeholder")
assert.True(t, strings.Contains(commentText, "*No manual checks match this pull request.*"), "should contains manual check placeholder")
assert.False(t, strings.Contains(commentText, "All **Automated Checks** passed. ✅"), "should contains automated checks passed placeholder")
assert.Equal(t, 2, len(autoCheckSuccessLine.FindAllStringSubmatch(commentText, -1)), "wrong number of succeeded automatic check")
assert.Equal(t, 3+3, len(autoCheckFailLine.FindAllStringSubmatch(commentText, -1)), "wrong number of failed automatic check")

content.ManualRules = manualRules
commentText, err = generateComment(content)
assert.Nil(t, err, fmt.Sprintf("error is not nil: %v", err))
assert.False(t, strings.Contains(commentText, "*No automated checks match this pull request.*"), "should not contains automated check placeholder")
assert.False(t, strings.Contains(commentText, "*No manual checks match this pull request.*"), "should not contains manual check placeholder")
assert.False(t, strings.Contains(commentText, "All **Automated Checks** passed. ✅"), "should contains automated checks passed placeholder")

manualChecks := getCommentManualChecks(commentText)
assert.Equal(t, len(manualChecks), len(manualRules), "wrong number of manual checks found")
Expand Down
9 changes: 9 additions & 0 deletions contribs/github-bot/internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ type ManualCheck struct {
Teams Teams // Members of these teams can check the checkbox to make the check pass.
}

// This is the description for a persistent rule with a non-standard behavior
// that allow maintainer to force the "success" state of the CI check
const ForceSkipDescription = "**SKIP**: Do not block the CI for this PR"

// 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) {
Expand Down Expand Up @@ -53,6 +57,11 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) {
}

manual := []ManualCheck{
{
// WARN: Do not edit this special rule which must remain persistent.
Description: ForceSkipDescription,
If: c.Always(),
},
{
Description: "The pull request description provides enough details",
If: c.Not(c.AuthorInTeam(gh, "core-contributors")),
Expand Down

0 comments on commit 8e7fb50

Please sign in to comment.