Change the way docs approvals work

Docs PRs are consistently delayed due to the the fact that we currently
require two approvals to come from a small team of people who are all
very busy.

To mitigate this, we will treat PRs that touch docs similarly to PRs
that touch code. The new behavior is:

1. PRs continue to require two approvals, whether they touch code, docs,
   or both.
2. At least one of these two approvals must come from a code "owner"
   (otherwise known as a "group 1 reviewer"). This becomes the case
   whether the PR touches docs or not.
3. If the PR does not touch docs, then approvals from the docs reviewers
   don't count towards the 2 approval limit.
4. If the PR does touch docs, then the docs reviewers are added to group
   2 reviewers. Their reviews count towards the two review limit, but
   their reviews are not required in order to merge the PR.

Author: zmb3

bot/go.mod

@@ -1,6 +1,6 @@
 module github.com/gravitational/shared-workflows/bot
 
-go 1.21
+go 1.23
 
 require (
 	github.com/google/go-github/v37 v37.0.0

bot/internal/bot/backport_test.go

@@ -44,7 +44,7 @@ func TestFindBranches(t *testing.T) {
 func TestBackport(t *testing.T) {
 	buildTestBot := func(github Client) (*Bot, context.Context) {
 		r, _ := review.New(&review.Config{
-			CoreReviewers:     map[string]review.Reviewer{"dev": review.Reviewer{}},
+			CoreReviewers:     map[string]review.Reviewer{"dev": {}},
 			CloudReviewers:    map[string]review.Reviewer{},
 			CodeReviewersOmit: map[string]bool{},
 			DocsReviewers:     map[string]review.Reviewer{},

bot/internal/bot/bloat.go

@@ -14,6 +14,7 @@ import (
 	"unicode/utf8"
 
 	"github.com/gravitational/trace"
+	"golang.org/x/exp/slices"
 )
 
 const (
@@ -89,14 +90,6 @@ func (b *Bot) BloatCheck(ctx context.Context, baseStats, current string, artifac
 		default:
 		}
 
-		skipped := false
-		for _, s := range skip {
-			if s == artifact {
-				skipped = true
-				break
-			}
-		}
-
 		stats, err := calculateChange(baseLookup, current, artifact)
 		if err != nil {
 			return err
@@ -105,7 +98,7 @@ func (b *Bot) BloatCheck(ctx context.Context, baseStats, current string, artifac
 		log.Printf("artifact %s has a current size of %d", artifact, stats.currentSize)
 
 		status := "✅"
-		if skipped {
+		if skipped := slices.Contains(skip, artifact); skipped {
 			status += " skipped by admin"
 		} else {
 			if stats.diff > int64(warnThreshold) {
@@ -150,13 +143,6 @@ func renderMarkdownTable(w io.Writer, data map[string]result) error {
 		padding[v] = len(v)
 	}
 
-	max := func(a, b int) int {
-		if a > b {
-			return a
-		}
-		return b
-	}
-
 	// get the largest item from the title or items in the column to determine
 	// the actual padding
 	for k, column := range data {

bot/internal/bot/check.go

@@ -20,8 +20,10 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"slices"
 	"strings"
 
+	"github.com/gravitational/shared-workflows/bot/internal/github"
 	"github.com/gravitational/shared-workflows/bot/internal/review"
 	"github.com/gravitational/trace"
 )
@@ -87,13 +89,9 @@ func (b *Bot) Check(ctx context.Context) error {
 			b.c.Environment.Repository,
 			b.c.Environment.Number,
 		)
-		commentExists := false
-		for _, c := range comments {
-			if c.Body == comment {
-				commentExists = true
-				break
-			}
-		}
+		commentExists := slices.ContainsFunc(comments, func(c github.Comment) bool {
+			return c.Body == comment
+		})
 		if !commentExists {
 			if err := b.c.GitHub.CreateComment(ctx,
 				b.c.Environment.Organization,
@@ -119,15 +117,6 @@ func (b *Bot) Check(ctx context.Context) error {
 	return nil
 }
 
-func contains(ss []string, s string) bool {
-	for i := range ss {
-		if ss[i] == s {
-			return true
-		}
-	}
-	return false
-}
-
 // checkDoNotMerge checks if the PR has "do-not-merge" label on it.
 func (b *Bot) checkDoNotMerge(ctx context.Context) error {
 	pull, err := b.c.GitHub.GetPullRequest(ctx,
@@ -138,7 +127,7 @@ func (b *Bot) checkDoNotMerge(ctx context.Context) error {
 		return trace.Wrap(err)
 	}
 
-	if contains(pull.UnsafeLabels, doNotMergeLabel) {
+	if slices.Contains(pull.UnsafeLabels, doNotMergeLabel) {
 		return trace.BadParameter("the pull request is marked as %v", doNotMergeLabel)
 	}
 
@@ -225,7 +214,7 @@ func (b *Bot) reviewersToDismiss(ctx context.Context) ([]string, error) {
 }
 
 const (
-	// doNotMergeLabel is the name of the Github label that is put on PRs
+	// doNotMergeLabel is the name of the GitHub label that is put on PRs
 	// to prevent them from merging.
 	doNotMergeLabel = "do-not-merge"
 )

bot/internal/bot/skip.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"github.com/gravitational/trace"
+	"golang.org/x/exp/slices"
 )
 
 // skipItems finds any comments from an admin with the skipPrefix
@@ -30,7 +31,7 @@ func (b *Bot) skipItems(ctx context.Context, skipPrefix string) ([]string, error
 
 	admins := b.c.Review.GetAdminCheckers(b.c.Environment.Author)
 	for _, c := range comments {
-		if !contains(admins, c.Author) {
+		if !slices.Contains(admins, c.Author) {
 			log.Printf("ignoring comment from non-admin %v", c.Author)
 			continue
 		}

bot/internal/review/review.go

@@ -52,7 +52,7 @@ var (
 	// singleApproverPaths defines paths in cloud or core repos that only require a single approver.
 	// The map key is the repo (cloud|teleport|teleport.e) and the value is a list of paths.
 	singleApproverPaths = map[string][]string{
-		"cloud": []string{
+		"cloud": {
 			"deploy/fluxcd/config/values.yaml",
 			"deploy/fluxcd/config/*/values.yaml",
 			"deploy/fluxcd/config/*/*/values.yaml",
@@ -68,7 +68,7 @@ var (
 	// require a single approver. The map key is the repo (cloud|teleport|teleport.e) and the
 	// value is a list of authors. BOTS ONLY - DO NOT INCLUDE EMPLOYEE GITHUB HANDLES.
 	singleApproverAuthors = map[string][]string{
-		"cloud": []string{Dependabot, DependabotBatcher, RenovateBotPrivate, RenovateBotPublic},
+		"cloud": {Dependabot, DependabotBatcher, RenovateBotPrivate, RenovateBotPublic},
 	}
 )
 
@@ -255,12 +255,12 @@ func (r *Assignments) getDocsReviewers(e *env.Environment, files []github.PullRe
 	// the changed docs files. If so, add them as docs reviewers.
 	repoReviewers := r.repoReviewers(e)
 	a, b := getReviewerSets(e.Author, repoReviewers, r.c.CodeReviewersOmit)
-	prefCodeReviewers := r.getAllPreferredReviewers(repoReviewers, append(a, b...), files)
+	preferredCodeReviewers := r.getAllPreferredReviewers(repoReviewers, append(a, b...), files)
 
 	// Get the docs reviewer pool, which does not depend on the files
 	// changed by a pull request.
 	docsA, docsB := getReviewerSets(e.Author, r.c.DocsReviewers, r.c.DocsReviewersOmit)
-	reviewers := append(prefCodeReviewers, append(docsA, docsB...)...)
+	reviewers := append(preferredCodeReviewers, append(docsA, docsB...)...)
 
 	// If no docs reviewers were assigned, assign admin reviews.
 	if len(reviewers) == 0 {
@@ -438,20 +438,17 @@ func (r *Assignments) CheckInternal(e *env.Environment, reviews []github.Review,
 	switch {
 	case changes.Docs && changes.Code:
 		log.Printf("Check: Found docs and code changes.")
-		if err := r.checkInternalDocsReviews(e, reviews, files); err != nil {
-			return trace.Wrap(err)
-		}
-		if err := r.checkInternalCodeReviews(e, changes, reviews); err != nil {
+		if err := r.checkInternalReviews(e, changes, reviews, files); err != nil {
 			return trace.Wrap(err)
 		}
 	case !changes.Docs && changes.Code:
 		log.Printf("Check: Found code changes.")
-		if err := r.checkInternalCodeReviews(e, changes, reviews); err != nil {
+		if err := r.checkInternalReviews(e, changes, reviews, files); err != nil {
 			return trace.Wrap(err)
 		}
 	case changes.Docs && !changes.Code:
 		log.Printf("Check: Found docs changes.")
-		if err := r.checkInternalDocsReviews(e, reviews, files); err != nil {
+		if err := r.checkInternalReviews(e, changes, reviews, files); err != nil {
 			return trace.Wrap(err)
 		}
 	// Strange state, an empty commit? Check admins.
@@ -478,23 +475,18 @@ func (r *Assignments) checkInternalReleaseReviews(reviews []github.Review) error
 	return trace.BadParameter("requires at least one approval from %v", reviewers)
 }
 
-// checkInternalDocsReviews checks whether docs review requirements are satisfied
+// checkInternalReviews checks whether review requirements are satisfied
 // for a PR authored by an internal employee
-func (r *Assignments) checkInternalDocsReviews(e *env.Environment, reviews []github.Review, files []github.PullRequestFile) error {
-	reviewers := r.getDocsReviewers(e, files)
+func (r *Assignments) checkInternalReviews(e *env.Environment, changes env.Changes, reviews []github.Review, files []github.PullRequestFile) error {
+	setA, setB := getReviewerSets(e.Author, r.repoReviewers(e), r.c.CodeReviewersOmit)
 
-	if check(reviewers, reviews) {
-		return nil
+	// If this PR touches docs, then approvals from docs reviewers also count.
+	// Add them to set B, as docs reviewers are not required so long as we get
+	// the appropriate number of approvals.
+	if changes.Docs {
+		setB = append(setB, r.getDocsReviewers(e, files)...)
 	}
 
-	return trace.BadParameter("requires at least one approval from %v", reviewers)
-}
-
-// checkInternalCodeReviews checks whether code review requirements are satisfied
-// for a PR authored by an internal employee
-func (r *Assignments) checkInternalCodeReviews(e *env.Environment, changes env.Changes, reviews []github.Review) error {
-	setA, setB := getReviewerSets(e.Author, r.repoReviewers(e), r.c.CodeReviewersOmit)
-
 	// PRs can be approved if you either have multiple code owners that approve
 	// or code owner and code reviewer. An exception is for PRs that
 	// only modify paths that require a single approver.

bot/internal/review/review_test.go

@@ -434,7 +434,7 @@ func TestGetCodeReviewerSets(t *testing.T) {
 				Author:     test.author,
 			}
 			changes := env.Changes{ApproverCount: env.DefaultApproverCount}
-			require.ErrorContains(t, test.assignments.checkInternalCodeReviews(e, changes, nil),
+			require.ErrorContains(t, test.assignments.checkInternalReviews(e, changes, nil, nil),
 				"at least one approval required from each set")
 
 			setA, setB := test.assignments.getCodeReviewerSets(e)
@@ -683,6 +683,7 @@ func TestCheckInternal(t *testing.T) {
 			// Docs.
 			DocsReviewers: map[string]Reviewer{
 				"7": {Owner: true},
+				"8": {Owner: false},
 			},
 			DocsReviewersOmit: map[string]bool{},
 			CodeReviewersOmit: map[string]bool{},
@@ -738,16 +739,28 @@ func TestCheckInternal(t *testing.T) {
 			result: false,
 		},
 		{
-			desc:       "docs-only-docs-approval-success",
+			desc:       "docs-only-two-approval-success",
 			repository: "teleport",
 			author:     "4",
 			reviews: []github.Review{
-				{Author: "7", State: Approved},
+				{Author: "1", State: Approved},
+				{Author: "8", State: Approved},
 			},
 			docs:   true,
 			code:   false,
 			result: true,
 		},
+		{
+			desc:       "docs-only-docs-approval-fail",
+			repository: "teleport",
+			author:     "4",
+			reviews: []github.Review{
+				{Author: "7", State: Approved},
+			},
+			docs:   true,
+			code:   false,
+			result: false,
+		},
 		{
 			desc:       "code-only-no-reviews-fail",
 			repository: "teleport",
@@ -850,9 +863,10 @@ func TestCheckInternal(t *testing.T) {
 				{Author: "3", State: Approved},
 				{Author: "4", State: Approved},
 			},
-			docs:   true,
-			code:   true,
-			result: false,
+			docs: true,
+			code: true,
+			// docs reviewers count towards required approvals, but are not required
+			result: true,
 		},
 		{
 			desc:       "docs-and-code-docs-and-code-approval-success",