Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(github-bot): refactor comment + add force skip #3311

Merged
merged 1 commit into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
go func(pr *github.PullRequest) {
defer wg.Done()
commentContent := CommentContent{}
commentContent.allSatisfied = true
commentContent.AutoAllSatisfied = true
commentContent.ManualAllSatisfied = true

Check warning on line 105 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L104-L105

Added lines #L104 - L105 were not covered by tests

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

Check warning on line 124 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L124

Added line #L124 was not covered by tests
}

c.ConditionDetails = ifDetails.String()
Expand Down Expand Up @@ -160,8 +161,14 @@
},
)

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

Check warning on line 171 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L164-L171

Added lines #L164 - L171 were not covered by tests
}
}

Expand Down Expand Up @@ -224,9 +231,20 @@
}

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)
}

Check warning on line 239 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L234-L239

Added lines #L234 - L239 were not covered by tests

if commentContent.ManualAllSatisfied {
logger.Infof("%s All manual checks are satisfied\n", utils.Success)

Check warning on line 242 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L241-L242

Added lines #L241 - L242 were not covered by tests
} else {
logger.Infof("%s Not all requirements are satisfied\n", utils.Fail)
logger.Infof("%s Some manual checks are not satisfied\n", utils.Fail)
}

Check warning on line 245 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L244-L245

Added lines #L244 - L245 were not covered by tests

if commentContent.ForceSkip {
logger.Infof("%s Bot checks are force skipped\n", utils.Success)

Check warning on line 248 in contribs/github-bot/internal/check/check.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/check.go#L247-L248

Added lines #L247 - L248 were not covered by tests
}
}
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 @@
// 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 @@
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 @@
// 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 @@
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."

Check warning on line 267 in contribs/github-bot/internal/check/comment.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/comment.go#L266-L267

Added lines #L266 - L267 were not covered by tests
)

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."

Check warning on line 274 in contribs/github-bot/internal/check/comment.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/check/comment.go#L270-L274

Added lines #L270 - L274 were not covered by tests
}

// 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 @@
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 @@
}

manual := []ManualCheck{
{
// WARN: Do not edit this special rule which must remain persistent.
Description: ForceSkipDescription,
If: c.Always(),
},

Check warning on line 64 in contribs/github-bot/internal/config/config.go

View check run for this annotation

Codecov / codecov/patch

contribs/github-bot/internal/config/config.go#L60-L64

Added lines #L60 - L64 were not covered by tests
{
Description: "The pull request description provides enough details",
If: c.Not(c.AuthorInTeam(gh, "core-contributors")),
Expand Down
Loading