Merge pull request #27052 from chaodaiG/revert-tide-interface

Revert "Tide Controller interacts with GitHub by interfaces"

Author: k8s-ci-robot

prow/config/prow-config-documented.yaml

@@ -1158,8 +1158,6 @@ tide:
     # Special values:
     # 0 => unlimited batch size
     # -1 => batch merging disabled :(
-
-    # This is suitable for any source code provider, so keep it global.
     batch_size_limit:
         "": 0
 
@@ -1266,8 +1264,6 @@ tide:
     # eligible. Continuing on an old batch allows to re-use all existing test results whereas
     # starting a new one requires to start new instances of all tests.
     # Use '*' as key to set this globally. Defaults to true.
-
-    # This is suitable for any source code provider, so keep it global.
     prioritize_existing_batches:
         "": false
 

prow/config/tide.go

@@ -155,8 +155,6 @@ type Tide struct {
 	// Special values:
 	//  0 => unlimited batch size
 	// -1 => batch merging disabled :(
-	//
-	// This is suitable for any source code provider, so keep it global.
 	BatchSizeLimitMap map[string]int `json:"batch_size_limit,omitempty"`
 
 	// Priority is an ordered list of sets of labels that would be prioritized before other PRs
@@ -169,8 +167,6 @@ type Tide struct {
 	// eligible. Continuing on an old batch allows to re-use all existing test results whereas
 	// starting a new one requires to start new instances of all tests.
 	// Use '*' as key to set this globally. Defaults to true.
-	//
-	// This is suitable for any source code provider, so keep it global.
 	PrioritizeExistingBatchesMap map[string]bool `json:"prioritize_existing_batches,omitempty"`
 
 	// DisplayAllQueriesInStatus controls if Tide should mention all queries in the status it

prow/tide/codereview.go

@@ -19,18 +19,9 @@ package tide
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
-	"strconv"
-	"sync"
 	"time"
 
 	"github.com/sirupsen/logrus"
-	utilerrors "k8s.io/apimachinery/pkg/util/errors"
-	prowapi "k8s.io/test-infra/prow/apis/prowjobs/v1"
-	"k8s.io/test-infra/prow/config"
-	"k8s.io/test-infra/prow/git/types"
-	"k8s.io/test-infra/prow/git/v2"
-	"k8s.io/test-infra/prow/tide/blockers"
 )
 
 // CodeReviewForDeck contains superset of data from CodeReviewCommon, it's meant
@@ -170,140 +161,3 @@ func CodeReviewCommonFromPullRequest(pr *PullRequest) *CodeReviewCommon {
 
 	return crc
 }
-
-// provider is the interface implemented by each source code
-// providers, such as GitHub and Gerrit.
-type provider interface {
-	Query() (map[string]CodeReviewCommon, error)
-	blockers() (blockers.Blockers, error)
-	isAllowedToMerge(crc *CodeReviewCommon) (string, error)
-	GetRef(org, repo, ref string) (string, error)
-	// headContexts returns Contexts from all presubmit requirements.
-	// Tide needs to know whether a PR passed all tests or not, this includes
-	// prow jobs, but also any external tests that are required by GitHub branch
-	// protection, for example GH actions. For GitHub these are all reflected on
-	// status contexts, and more importantly each prowjob is a context. For
-	// Gerrit we can transform every prow jobs into a context, and mark it
-	// optional if the prowjob doesn't vote on label that's required for
-	// merging. And also transform any other label that is not voted by prow
-	// into a context.
-	headContexts(pr *CodeReviewCommon) ([]Context, error)
-	mergePRs(sp subpool, prs []CodeReviewCommon, dontUpdateStatus *threadSafePRSet) error
-	GetTideContextPolicy(gitClient git.ClientFactory, org, repo, branch string, baseSHAGetter config.RefGetter, headSHA string) (contextChecker, error)
-	refsForJob(sp subpool, prs []CodeReviewCommon) prowapi.Refs
-	prMergeMethod(crc *CodeReviewCommon) (types.PullRequestMergeType, error)
-}
-
-// GitHubProvider implements provider, used by tide Controller for
-// interacting directly with GitHub.
-//
-// Tide Controller should only use GitHubProvider for communicating with GitHub.
-type GitHubProvider struct {
-	cfg                config.Getter
-	ghc                githubClient
-	usesGitHubAppsAuth bool
-
-	*mergeChecker
-	logger *logrus.Entry
-}
-
-func (gi *GitHubProvider) blockers() (blockers.Blockers, error) {
-	label := gi.cfg().Tide.BlockerLabel
-	if label == "" {
-		return blockers.Blockers{}, nil
-	}
-
-	gi.logger.WithField("blocker_label", label).Debug("Searching for blocker issues")
-	orgExcepts, repos := gi.cfg().Tide.Queries.OrgExceptionsAndRepos()
-	orgs := make([]string, 0, len(orgExcepts))
-	for org := range orgExcepts {
-		orgs = append(orgs, org)
-	}
-	orgRepoQuery := orgRepoQueryStrings(orgs, repos.UnsortedList(), orgExcepts)
-	return blockers.FindAll(gi.ghc, gi.logger, label, orgRepoQuery, gi.usesGitHubAppsAuth)
-}
-
-// Query gets all open PRs based on tide configuration.
-func (gi *GitHubProvider) Query() (map[string]CodeReviewCommon, error) {
-	lock := sync.Mutex{}
-	wg := sync.WaitGroup{}
-	prs := make(map[string]CodeReviewCommon)
-	var errs []error
-	for i, query := range gi.cfg().Tide.Queries {
-
-		// Use org-sharded queries only when GitHub apps auth is in use
-		var queries map[string]string
-		if gi.usesGitHubAppsAuth {
-			queries = query.OrgQueries()
-		} else {
-			queries = map[string]string{"": query.Query()}
-		}
-
-		for org, q := range queries {
-			org, q, i := org, q, i
-			wg.Add(1)
-			go func() {
-				defer wg.Done()
-				results, err := gi.search(gi.ghc.QueryWithGitHubAppsSupport, gi.logger, q, time.Time{}, time.Now(), org)
-
-				resultString := "success"
-				if err != nil {
-					resultString = "error"
-				}
-				tideMetrics.queryResults.WithLabelValues(strconv.Itoa(i), org, resultString).Inc()
-
-				lock.Lock()
-				defer lock.Unlock()
-				if err != nil && len(results) == 0 {
-					gi.logger.WithField("query", q).WithError(err).Warn("Failed to execute query.")
-					errs = append(errs, fmt.Errorf("query %d, err: %w", i, err))
-					return
-				}
-				if err != nil {
-					gi.logger.WithError(err).WithField("query", q).Warning("found partial results")
-				}
-
-				for _, pr := range results {
-					crc := CodeReviewCommonFromPullRequest(&pr)
-					prs[prKey(crc)] = *crc
-				}
-			}()
-		}
-	}
-	wg.Wait()
-
-	return prs, utilerrors.NewAggregate(errs)
-}
-
-func (gi *GitHubProvider) GetRef(org, repo, ref string) (string, error) {
-	return gi.ghc.GetRef(org, repo, ref)
-}
-
-func (gi *GitHubProvider) GetTideContextPolicy(gitClient git.ClientFactory, org, repo, branch string, baseSHAGetter config.RefGetter, headSHA string) (contextChecker, error) {
-	return gi.cfg().GetTideContextPolicy(gitClient, org, repo, branch, baseSHAGetter, headSHA)
-}
-
-func (gi *GitHubProvider) refsForJob(sp subpool, prs []CodeReviewCommon) prowapi.Refs {
-	refs := prowapi.Refs{
-		Org:     sp.org,
-		Repo:    sp.repo,
-		BaseRef: sp.branch,
-		BaseSHA: sp.sha,
-	}
-	for _, pr := range prs {
-		refs.Pulls = append(
-			refs.Pulls,
-			prowapi.Pull{
-				Number: pr.Number,
-				Title:  pr.Title,
-				Author: string(pr.AuthorLogin),
-				SHA:    pr.HeadRefOID,
-			},
-		)
-	}
-	return refs
-}
-
-func (gi *GitHubProvider) prMergeMethod(crc *CodeReviewCommon) (types.PullRequestMergeType, error) {
-	return gi.mergeChecker.prMergeMethod(gi.cfg().Tide, crc)
-}

prow/tide/search.go

@@ -40,7 +40,7 @@ func floor(t time.Time) time.Time {
 	return t
 }
 
-func (gi *GitHubProvider) search(query querier, log *logrus.Entry, q string, start, end time.Time, org string) ([]PullRequest, error) {
+func search(query querier, log *logrus.Entry, q string, start, end time.Time, org string) ([]CodeReviewCommon, error) {
 	start = floor(start)
 	end = floor(end)
 	log = log.WithFields(logrus.Fields{
@@ -56,7 +56,7 @@ func (gi *GitHubProvider) search(query querier, log *logrus.Entry, q string, sta
 	}
 
 	var totalCost, remaining int
-	var ret []PullRequest
+	var ret []CodeReviewCommon
 	var sq searchQuery
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancel()
@@ -71,7 +71,7 @@ func (gi *GitHubProvider) search(query querier, log *logrus.Entry, q string, sta
 		totalCost += int(sq.RateLimit.Cost)
 		remaining = int(sq.RateLimit.Remaining)
 		for _, n := range sq.Search.Nodes {
-			ret = append(ret, n.PullRequest)
+			ret = append(ret, *CodeReviewCommonFromPullRequest(&n.PullRequest))
 		}
 		if !sq.Search.PageInfo.HasNextPage {
 			break

prow/tide/search_test.go

@@ -136,7 +136,6 @@ func TestSearch(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			client := &GitHubProvider{}
 			var i int
 			querier := func(_ context.Context, result interface{}, actual map[string]interface{}, _ string) error {
 				expected := map[string]interface{}{
@@ -156,7 +155,7 @@ func TestSearch(t *testing.T) {
 				*ret = sq
 				return nil
 			}
-			prs, err := client.search(querier, logrus.WithField("test", tc.name), q, tc.start, tc.end, "")
+			prs, err := search(querier, logrus.WithField("test", tc.name), q, tc.start, tc.end, "")
 			switch {
 			case err != nil:
 				if !tc.err {
@@ -165,8 +164,12 @@ func TestSearch(t *testing.T) {
 			case tc.err:
 				t.Errorf("failed to receive expected error")
 			}
-
-			if !reflect.DeepEqual(tc.expected, prs) {
+			// Always check prs because we might return some results on error
+			var expectedCrcs []CodeReviewCommon
+			for _, pr := range tc.expected {
+				expectedCrcs = append(expectedCrcs, *CodeReviewCommonFromPullRequest(&pr))
+			}
+			if !reflect.DeepEqual(expectedCrcs, prs) {
 				t.Errorf("prs do not match:\n%s", diff.ObjectReflectDiff(tc.expected, prs))
 			}
 		})

prow/tide/status.go

@@ -62,16 +62,16 @@ type storedState struct {
 	PreviousQuery string
 }
 
-// statusController is a goroutine runs in the background
 type statusController struct {
 	pjClient           ctrlruntimeclient.Client
 	logger             *logrus.Entry
 	config             config.Getter
-	ghProvider         *GitHubProvider
 	ghc                githubClient
 	gc                 git.ClientFactory
 	usesGitHubAppsAuth bool
 
+	mergeChecker *mergeChecker
+
 	// shutDown is used to signal to the main controller that the statusController
 	// has completed processing after newPoolPending is closed.
 	shutDown chan bool
@@ -278,7 +278,7 @@ func (sc *statusController) expectedStatus(log *logrus.Entry, queryMap *config.Q
 
 	repo := config.OrgRepo{Org: crc.Org, Repo: crc.Repo}
 
-	if reason, err := sc.ghProvider.isAllowedToMerge(crc); err != nil {
+	if reason, err := sc.mergeChecker.isAllowed(crc); err != nil {
 		return "", "", fmt.Errorf("error checking if merge is allowed: %w", err)
 	} else if reason != "" {
 		log.WithField("reason", reason).Debug("The PR is not mergeable")
@@ -414,7 +414,7 @@ func (sc *statusController) setStatuses(all []CodeReviewCommon, pool map[string]
 	process := func(pr *CodeReviewCommon) {
 		processed.Insert(prKey(pr))
 		log := sc.logger.WithFields(pr.logFields())
-		contexts, err := sc.ghProvider.headContexts(pr)
+		contexts, err := headContexts(log, sc.ghc, pr)
 		if err != nil {
 			log.WithError(err).Error("Getting head commit status contexts, skipping...")
 			return
@@ -657,7 +657,7 @@ func (sc *statusController) search() []CodeReviewCommon {
 			}
 			sc.storedStateLock.Unlock()
 
-			result, err := sc.ghProvider.search(sc.ghc.QueryWithGitHubAppsSupport, sc.logger, query, latestPR.Time, now, org)
+			result, err := search(sc.ghc.QueryWithGitHubAppsSupport, sc.logger, query, latestPR.Time, now, org)
 			log.WithField("duration", time.Since(now).String()).WithField("result_count", len(result)).Debug("Searched for open PRs.")
 
 			func() {
@@ -669,7 +669,7 @@ func (sc *statusController) search() []CodeReviewCommon {
 					log.Debug("no new results")
 					return
 				}
-				latest := result[len(result)-1].UpdatedAt
+				latest := result[len(result)-1].UpdatedAtTime
 				if latest.IsZero() {
 					log.Debug("latest PR has zero time")
 					return
@@ -684,10 +684,7 @@ func (sc *statusController) search() []CodeReviewCommon {
 			lock.Lock()
 			defer lock.Unlock()
 
-			for _, pr := range result {
-				pr := pr
-				prs = append(prs, *CodeReviewCommonFromPullRequest(&pr))
-			}
+			prs = append(prs, result...)
 			errs = append(errs, err)
 		}()
 

prow/tide/status_test.go

@@ -1222,14 +1222,11 @@ func TestSetStatusRespectsRequiredContexts(t *testing.T) {
 	ca.Set(&config.Config{})
 
 	sc := &statusController{
-		logger:   log,
-		ghc:      fghc,
-		config:   ca.Config,
-		pjClient: fakectrlruntimeclient.NewFakeClient(),
-		ghProvider: &GitHubProvider{
-			ghc:          fghc,
-			mergeChecker: newMergeChecker(ca.Config, fghc),
-		},
+		logger:       log,
+		ghc:          fghc,
+		config:       ca.Config,
+		pjClient:     fakectrlruntimeclient.NewFakeClient(),
+		mergeChecker: newMergeChecker(ca.Config, fghc),
 		statusUpdate: &statusUpdate{
 			dontUpdateStatus: &threadSafePRSet{},
 			newPoolPending:   make(chan bool),