Skip to content

Commit

Permalink
Merge pull request #2839 from hongkailiu/retest_fix_enabled_pr
Browse files Browse the repository at this point in the history 
retester: check if a PR is enabled earlier
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed May 27, 2022
2 parents d166f73 + 9977469 commit c09e588
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 118 deletions.
4 changes: 2 additions & 2 deletions cmd/retester/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ func gatherOptions() options {
fs.StringVar(&intervalRaw, "interval", "1h", "Parseable duration string that specifies the sync period")
fs.StringVar(&o.cacheFile, "cache-file", "", "File to persist cache. No persistence of cache if not set")
fs.StringVar(&cacheRecordAgeRaw, "cache-record-age", "168h", "Parseable duration string that specifies how long a cache record lives in cache after the last time it was considered")
fs.Var(&o.enableOnRepos, "enable-on-repo", "Repository is saved in list. It can be used more than once, the result is a list of repositories where we start commenting instead of logging")
fs.Var(&o.enableOnOrgs, "enable-on-org", "Organization is saved in list. It can be used more than once, the result is a list of organizations (owners) where we start commenting instead of logging")
fs.Var(&o.enableOnRepos, "enable-on-repo", "Repository that the retester is enabled on, e.g., 'openshift/ci-tools'. It can be used more than once.")
fs.Var(&o.enableOnOrgs, "enable-on-org", "Organization that the retester is enabled on, e.g., 'openshift'. It can be used more than once.")

for _, group := range []flagutil.OptionGroup{&o.github, &o.config} {
group.AddFlags(fs)
Expand Down
56 changes: 33 additions & 23 deletions cmd/retester/retester.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,21 @@ type retestController struct {
usesGitHubApp bool
backoff *backoffCache

commentOnRepos sets.String
commentOnOrgs sets.String
enableOnRepos sets.String
enableOnOrgs sets.String
}

func newController(ghClient githubClient, cfg config.Getter, gitClient git.ClientFactory, usesApp bool, cacheFile string, cacheRecordAge time.Duration, enableOnRepos prowflagutil.Strings, enableOnOrgs prowflagutil.Strings) *retestController {
logger := logrus.NewEntry(logrus.StandardLogger())
ret := &retestController{
ghClient: ghClient,
gitClient: gitClient,
configGetter: cfg,
logger: logger,
usesGitHubApp: usesApp,
backoff: &backoffCache{cache: map[string]*PullRequest{}, file: cacheFile, cacheRecordAge: cacheRecordAge, logger: logger},
commentOnRepos: enableOnRepos.StringSet(),
commentOnOrgs: enableOnOrgs.StringSet(),
ghClient: ghClient,
gitClient: gitClient,
configGetter: cfg,
logger: logger,
usesGitHubApp: usesApp,
backoff: &backoffCache{cache: map[string]*PullRequest{}, file: cacheFile, cacheRecordAge: cacheRecordAge, logger: logger},
enableOnRepos: enableOnRepos.StringSet(),
enableOnOrgs: enableOnOrgs.StringSet(),
}
if err := ret.backoff.loadFromDisk(); err != nil {
logger.WithError(err).Warn("Failed to load backoff cache from disk")
Expand All @@ -149,6 +149,9 @@ func (c *retestController) sync() error {
logrus.Infof("Candidate PR: %s", prUrl(pr))
}

candidates = c.enabledPRs(candidates)
logrus.Infof("Remaining %d candidates for retest (from an enabled org or repo)", len(candidates))

candidates, err = c.atLeastOneRequiredJob(candidates)
if err != nil {
return fmt.Errorf("failed to filter candidate PRs that have at least one required job: %w", err)
Expand Down Expand Up @@ -215,15 +218,10 @@ func (b *backoffCache) check(pr tide.PullRequest, baseSha string) (retestBackoff
return retestBackoffRetest, fmt.Sprintf("Remaining retests: %d against base HEAD %s and %d for PR HEAD %s in total", maxRetestsForShaAndBase-record.RetestsForBaseSha, record.BaseSha, maxRetestsForSha-record.RetestsForPrSha, record.PRSha)
}

func (c *retestController) createComment(pr tide.PullRequest, org, repoName, cmd, message string) {
repo := fmt.Sprintf("%s/%s", pr.Repository.Owner.Login, pr.Repository.Name)
if c.commentOnOrgs.Has(org) || c.commentOnRepos.Has(repo) {
comment := fmt.Sprintf("%s\n\n%s\n", cmd, message)
if err := c.ghClient.CreateComment(string(pr.Repository.Owner.Login), string(pr.Repository.Name), int(pr.Number), comment); err != nil {
c.logger.WithError(err).Error("failed to create a comment")
}
} else {
c.logger.Infof("%s: %s (%s)", prUrl(pr), cmd, message)
func (c *retestController) createComment(pr tide.PullRequest, cmd, message string) {
comment := fmt.Sprintf("%s\n\n%s\n", cmd, message)
if err := c.ghClient.CreateComment(string(pr.Repository.Owner.Login), string(pr.Repository.Name), int(pr.Number), comment); err != nil {
c.logger.WithField("comment", comment).WithError(err).Error("failed to create a comment")
}
}

Expand All @@ -234,16 +232,14 @@ func (c *retestController) retestOrBackoff(pr tide.PullRequest) error {
return err
}

org := string(pr.Repository.Owner.Login)
repoName := string(pr.Repository.Name)
action, message := c.backoff.check(pr, baseSha)
switch action {
case retestBackoffHold:
c.createComment(pr, org, repoName, "/hold", message)
c.createComment(pr, "/hold", message)
case retestBackoffPause:
c.logger.Infof("%s: %s (%s)", prUrl(pr), "no comment", message)
case retestBackoffRetest:
c.createComment(pr, org, repoName, "/retest-required", message)
c.createComment(pr, "/retest-required", message)
}
return nil
}
Expand Down Expand Up @@ -448,6 +444,20 @@ func (c *retestController) presubmitsForPRByContext(pr tide.PullRequest) map[str
return presubmits
}

func (c *retestController) enabledPRs(candidates map[string]tide.PullRequest) map[string]tide.PullRequest {
output := map[string]tide.PullRequest{}
for key, pr := range candidates {
org := string(pr.Repository.Owner.Login)
orgRepo := fmt.Sprintf("%s/%s", org, pr.Repository.Name)
if c.enableOnOrgs.Has(org) || c.enableOnRepos.Has(orgRepo) {
output[key] = pr
} else {
c.logger.Infof("PR %s is not from an enabled org or repo", key)
}
}
return output
}

// headContexts gets the status contexts for the commit with OID == pr.HeadRefOID
//
// First, we try to get this value from the commits we got with the PR query.
Expand Down
180 changes: 87 additions & 93 deletions cmd/retester/retester_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/shurcooL/githubv4"
"github.com/sirupsen/logrus"

prowflagutil "k8s.io/test-infra/prow/flagutil"
"k8s.io/apimachinery/pkg/util/sets"
github "k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/github/fakegithub"
"k8s.io/test-infra/prow/tide"
Expand All @@ -35,9 +35,7 @@ func (f *MyFakeClient) GetRef(owner, repo, ref string) (string, error) {
func TestRetestOrBackoff(t *testing.T) {
ghc := &MyFakeClient{fakegithub.NewFakeClient()}
var name githubv4.String = "repo"
var notEnabledRepo githubv4.String = "other-repo"
var owner githubv4.String = "org"
var notEnabledOwner githubv4.String = "other-org"
var fail githubv4.String = "failed test"
var num githubv4.Int = 123
var num2 githubv4.Int = 321
Expand All @@ -46,9 +44,6 @@ func TestRetestOrBackoff(t *testing.T) {
ghc.PullRequests = map[int]*github.PullRequest{123: &pr123, 321: &pr321}
logger := logrus.NewEntry(logrus.StandardLogger())

enableOnRepos := prowflagutil.NewStrings("org/repo")
enableOnOrgs := prowflagutil.NewStrings("org")

testCases := []struct {
name string
pr tide.PullRequest
Expand All @@ -68,32 +63,12 @@ func TestRetestOrBackoff(t *testing.T) {
}{Name: name, Owner: struct{ Login githubv4.String }{Login: owner}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnRepos: enableOnRepos.StringSet(),
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
},
expected: "/retest-required\n\nRemaining retests: 2 against base HEAD abcde and 8 for PR HEAD in total\n",
},
{
name: "no comment",
pr: tide.PullRequest{
Number: num2,
Author: struct{ Login githubv4.String }{Login: owner},
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: notEnabledRepo, Owner: struct{ Login githubv4.String }{Login: owner}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnRepos: enableOnRepos.StringSet(),
},
expected: "",
},
{
name: "failed test",
pr: tide.PullRequest{
Expand All @@ -103,72 +78,12 @@ func TestRetestOrBackoff(t *testing.T) {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: notEnabledRepo, Owner: struct{ Login githubv4.String }{Login: fail}},
}{Name: name, Owner: struct{ Login githubv4.String }{Login: fail}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnRepos: enableOnRepos.StringSet(),
},
expected: "",
expectedError: fmt.Errorf("failed"),
},
{
name: "basic case org",
pr: tide.PullRequest{
Number: num,
Author: struct{ Login githubv4.String }{Login: owner},
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: notEnabledRepo, Owner: struct{ Login githubv4.String }{Login: owner}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnOrgs: enableOnOrgs.StringSet(),
},
expected: "/retest-required\n\nRemaining retests: 2 against base HEAD abcde and 8 for PR HEAD in total\n",
},
{
name: "no comment org",
pr: tide.PullRequest{
Number: num2,
Author: struct{ Login githubv4.String }{Login: owner},
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: notEnabledRepo, Owner: struct{ Login githubv4.String }{Login: notEnabledOwner}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnOrgs: enableOnOrgs.StringSet(),
},
expected: "",
},
{
name: "failed test org",
pr: tide.PullRequest{
Number: num2,
Author: struct{ Login githubv4.String }{Login: fail},
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: notEnabledRepo, Owner: struct{ Login githubv4.String }{Login: fail}},
},
c: &retestController{
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
commentOnRepos: enableOnRepos.StringSet(),
commentOnOrgs: enableOnOrgs.StringSet(),
ghClient: ghc,
logger: logger,
backoff: &backoffCache{cache: map[string]*PullRequest{}, logger: logger},
},
expected: "",
expectedError: fmt.Errorf("failed"),
Expand All @@ -192,3 +107,82 @@ func TestRetestOrBackoff(t *testing.T) {
})
}
}

func TestEnabledPRs(t *testing.T) {
logger := logrus.NewEntry(logrus.StandardLogger())
testCases := []struct {
name string
c *retestController
candidates map[string]tide.PullRequest
expected map[string]tide.PullRequest
}{
{
name: "basic case",
c: &retestController{
enableOnRepos: sets.NewString("openshift/ci-tools"),
enableOnOrgs: sets.NewString("org-a"),
logger: logger,
},
candidates: map[string]tide.PullRequest{
"a": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "ci-tools", Owner: struct{ Login githubv4.String }{Login: "openshift"}},
},
"b": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "some-tools", Owner: struct{ Login githubv4.String }{Login: "openshift"}},
},
"c": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "some-tools", Owner: struct{ Login githubv4.String }{Login: "org-a"}},
},
"d": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "some-tools", Owner: struct{ Login githubv4.String }{Login: "org-b"}},
},
},
expected: map[string]tide.PullRequest{
"a": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "ci-tools", Owner: struct{ Login githubv4.String }{Login: "openshift"}},
},
"c": {
Number: 1,
Repository: struct {
Name githubv4.String
NameWithOwner githubv4.String
Owner struct{ Login githubv4.String }
}{Name: "some-tools", Owner: struct{ Login githubv4.String }{Login: "org-a"}},
},
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual := tc.c.enabledPRs(tc.candidates)
if diff := cmp.Diff(tc.expected, actual); diff != "" {
t.Errorf("%s differs from expected:\n%s", tc.name, diff)
}
})
}
}

0 comments on commit c09e588

Please sign in to comment.