diff --git a/prow/plugins/dco/dco.go b/prow/plugins/dco/dco.go index 96a330e7616c..4fab19aef41a 100644 --- a/prow/plugins/dco/dco.go +++ b/prow/plugins/dco/dco.go @@ -113,11 +113,11 @@ func filterTrustedUsers(gc gitHubClient, l *logrus.Entry, skipDCOCheckForCollabo untrustedCommits := make([]github.RepositoryCommit, 0, len(allCommits)) for _, commit := range allCommits { - trusted, err := trigger.TrustedUser(gc, !skipDCOCheckForCollaborators, trustedOrg, commit.Author.Login, org, repo) + trustedResponse, err := trigger.TrustedUser(gc, !skipDCOCheckForCollaborators, trustedOrg, commit.Author.Login, org, repo) if err != nil { return nil, fmt.Errorf("Error checking is member trusted: %v", err) } - if !trusted { + if !trustedResponse.IsTrusted { l.Debugf("Member %s is not trusted", commit.Author.Login) untrustedCommits = append(untrustedCommits, commit) } diff --git a/prow/plugins/retitle/retitle.go b/prow/plugins/retitle/retitle.go index 219860677606..1d9a51fae721 100644 --- a/prow/plugins/retitle/retitle.go +++ b/prow/plugins/retitle/retitle.go @@ -74,7 +74,8 @@ func handleGenericCommentEvent(pc plugins.Agent, e github.GenericCommentEvent) e ) return handleGenericComment(pc.GitHubClient, func(user string) (bool, error) { t := pc.PluginConfig.TriggerFor(org, repo) - return trigger.TrustedUser(pc.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo) + trustedResponse, err := trigger.TrustedUser(pc.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo) + return trustedResponse.IsTrusted, err }, pc.PluginConfig.Retitle.AllowClosedIssues, pc.Logger, e) } diff --git a/prow/plugins/trigger/generic-comment.go b/prow/plugins/trigger/generic-comment.go index 9f875dc7fefc..f2e1bce55e86 100644 --- a/prow/plugins/trigger/generic-comment.go +++ b/prow/plugins/trigger/generic-comment.go @@ -94,10 +94,12 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo } // Skip untrusted users comments. - trusted, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, commentAuthor, org, repo) + trustedResponse, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, commentAuthor, org, repo) if err != nil { return fmt.Errorf("error checking trust of %s: %v", commentAuthor, err) } + + trusted := trustedResponse.IsTrusted var l []github.Label if !trusted { // Skip untrusted PRs. diff --git a/prow/plugins/trigger/pull-request.go b/prow/plugins/trigger/pull-request.go index adc99deeac1f..994bd74dda06 100644 --- a/prow/plugins/trigger/pull-request.go +++ b/prow/plugins/trigger/pull-request.go @@ -71,7 +71,8 @@ func handlePR(c Client, trigger plugins.Trigger, pr github.PullRequestEvent) err // When a PR is opened, if the author is in the org then build it. // Otherwise, ask for "/ok-to-test". There's no need to look for previous // "/ok-to-test" comments since the PR was just opened! - member, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo) + trustedResponse, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo) + member := trustedResponse.IsTrusted if err != nil { return fmt.Errorf("could not check membership: %s", err) } @@ -307,9 +308,9 @@ func draftMsg(ghc githubClient, pr github.PullRequest) error { // If already known, GitHub labels should be provided to save tokens. Otherwise, it fetches them. func TrustedPullRequest(tprc trustedPullRequestClient, trigger plugins.Trigger, author, org, repo string, num int, l []github.Label) ([]github.Label, bool, error) { // First check if the author is a member of the org. - if orgMember, err := TrustedUser(tprc, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo); err != nil { + if trustedResponse, err := TrustedUser(tprc, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo); err != nil { return l, false, fmt.Errorf("error checking %s for trust: %v", author, err) - } else if orgMember { + } else if trustedResponse.IsTrusted { return l, true, nil } // Then check if PR has ok-to-test label diff --git a/prow/plugins/trigger/trigger.go b/prow/plugins/trigger/trigger.go index 6f899510ed8b..1d64154544a6 100644 --- a/prow/plugins/trigger/trigger.go +++ b/prow/plugins/trigger/trigger.go @@ -38,6 +38,33 @@ const ( PluginName = "trigger" ) +// untrustedReason represents a combination (by ORing the appropriate consts) of reasons +// why a user is not trusted by TrustedUser. It is used to generate messaging for users. +type untrustedReason int + +const ( + notMember untrustedReason = 1 << iota + notCollaborator + notSecondaryMember +) + +// String constructs a string explaining the reason for a user's denial of trust +// from untrustedReason as described above. +func (u untrustedReason) String() string { + var response string + if u¬Member != 0 { + response += "User is not a member of the org. " + } + if u¬Collaborator != 0 { + response += "User is not a collaborator. " + } + if u¬SecondaryMember != 0 { + response += "User is not a member of the trusted secondary org. " + } + response += "Satisfy at least one of these conditions to make the user trusted." + return response +} + func init() { plugins.RegisterGenericCommentHandler(PluginName, handleGenericCommentEvent, helpProvider) plugins.RegisterPullRequestHandler(PluginName, handlePullRequest, helpProvider) @@ -160,15 +187,26 @@ func handlePush(pc plugins.Agent, pe github.PushEvent) error { return handlePE(getClient(pc), pe) } +// TrustedUserResponse is a response from TrustedUser. It contains the boolean response for trust as well +// a reason for denial if the user is not trusted. +type TrustedUserResponse struct { + IsTrusted bool + // Reason contains the reason that a user is not trusted if IsTrusted is false + Reason string +} + // TrustedUser returns true if user is trusted in repo. // Trusted users are either repo collaborators, org members or trusted org members. -func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, org, repo string) (bool, error) { +func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, org, repo string) (TrustedUserResponse, error) { + errorResponse := TrustedUserResponse{IsTrusted: false} + okResponse := TrustedUserResponse{IsTrusted: true} + // First check if user is a collaborator, assuming this is allowed if !onlyOrgMembers { if ok, err := ghc.IsCollaborator(org, repo, user); err != nil { - return false, fmt.Errorf("error in IsCollaborator: %v", err) + return errorResponse, fmt.Errorf("error in IsCollaborator: %v", err) } else if ok { - return true, nil + return okResponse, nil } } @@ -176,22 +214,34 @@ func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, o // Next see if the user is an org member if member, err := ghc.IsMember(org, user); err != nil { - return false, fmt.Errorf("error in IsMember(%s): %v", org, err) + return errorResponse, fmt.Errorf("error in IsMember(%s): %v", org, err) } else if member { - return true, nil + return okResponse, nil } - // Determine if there is a second org to check + // Determine if there is a second org to check. If there is no secondary org or they are the same, the result + // is the same because the user already failed the check for the primary org. if trustedOrg == "" || trustedOrg == org { - return false, nil // No trusted org and/or it is the same + // the if/else is only to improve error messaging + if onlyOrgMembers { + return TrustedUserResponse{IsTrusted: false, Reason: notMember.String()}, nil // No trusted org and/or it is the same + } + return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notCollaborator).String()}, nil // No trusted org and/or it is the same } // Check the second trusted org. member, err := ghc.IsMember(trustedOrg, user) if err != nil { - return false, fmt.Errorf("error in IsMember(%s): %v", trustedOrg, err) + return errorResponse, fmt.Errorf("error in IsMember(%s): %v", trustedOrg, err) + } else if member { + return okResponse, nil + } + + // the if/else is only to improve error messaging + if onlyOrgMembers { + return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notSecondaryMember).String()}, nil } - return member, nil + return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notSecondaryMember | notCollaborator).String()}, nil } func skippedStatusFor(context string) github.Status { diff --git a/prow/plugins/trigger/trigger_test.go b/prow/plugins/trigger/trigger_test.go index 8b2966e5d6db..569024551ba7 100644 --- a/prow/plugins/trigger/trigger_test.go +++ b/prow/plugins/trigger/trigger_test.go @@ -270,6 +270,7 @@ func TestTrustedUser(t *testing.T) { repo string expectedTrusted bool + expectedReason string }{ { name: "user is member of trusted org", @@ -302,6 +303,7 @@ func TestTrustedUser(t *testing.T) { org: "kubernetes", repo: "kubernetes", expectedTrusted: false, + expectedReason: (notMember).String(), }, { name: "user is trusted org member", @@ -319,6 +321,27 @@ func TestTrustedUser(t *testing.T) { org: "kubernetes", repo: "kubernetes", expectedTrusted: false, + expectedReason: (notMember | notCollaborator).String(), + }, + { + name: "user is not org member or trusted org member", + onlyOrgMembers: false, + trustedOrg: "kubernetes-sigs", + user: "test-2", + org: "kubernetes", + repo: "kubernetes", + expectedTrusted: false, + expectedReason: (notMember | notCollaborator | notSecondaryMember).String(), + }, + { + name: "user is not org member or trusted org member, onlyOrgMembers true", + onlyOrgMembers: true, + trustedOrg: "kubernetes-sigs", + user: "test-2", + org: "kubernetes", + repo: "kubernetes", + expectedTrusted: false, + expectedReason: (notMember | notSecondaryMember).String(), }, } @@ -331,12 +354,15 @@ func TestTrustedUser(t *testing.T) { Collaborators: []string{"test-collaborator"}, } - trusted, err := TrustedUser(fc, tc.onlyOrgMembers, tc.trustedOrg, tc.user, tc.org, tc.repo) + trustedResponse, err := TrustedUser(fc, tc.onlyOrgMembers, tc.trustedOrg, tc.user, tc.org, tc.repo) if err != nil { t.Errorf("For case %s, didn't expect error from TrustedUser: %v", tc.name, err) } - if trusted != tc.expectedTrusted { - t.Errorf("For case %s, expect result: %v, but got: %v", tc.name, tc.expectedTrusted, trusted) + if trustedResponse.IsTrusted != tc.expectedTrusted { + t.Errorf("For case %s, expect trusted: %v, but got: %v", tc.name, tc.expectedTrusted, trustedResponse.IsTrusted) + } + if trustedResponse.Reason != tc.expectedReason { + t.Errorf("For case %s, expect trusted reason: %v, but got: %v", tc.name, tc.expectedReason, trustedResponse.Reason) } }) } diff --git a/prow/plugins/verify-owners/verify-owners.go b/prow/plugins/verify-owners/verify-owners.go index 4e32ab8df091..fd0a99c0619a 100644 --- a/prow/plugins/verify-owners/verify-owners.go +++ b/prow/plugins/verify-owners/verify-owners.go @@ -41,14 +41,19 @@ import ( const ( // PluginName defines this plugin's registered name. - PluginName = "verify-owners" - ownersFileName = "OWNERS" - ownersAliasesFileName = "OWNERS_ALIASES" - nonCollaboratorResponseFormat = `The following users are mentioned in %s file(s) but are not members of the %s org. - -Once all users have been added as [members](%s) of the org, you can trigger verification by writing ` + "`/verify-owners`" + ` in a comment.` + PluginName = "verify-owners" + ownersFileName = "OWNERS" + ownersAliasesFileName = "OWNERS_ALIASES" + untrustedResponseFormat = `The following users are mentioned in %s file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as [members](%s) of the %s org. You can then trigger verification by writing ` + "`/verify-owners`" + ` in a comment.` ) +type nonTrustedReasons struct { + // files is a list of files they are being added in + files []string + // triggerReason is the reason that trigger's TrustedUser responds with for a failed trust check + triggerReason string +} + var ( verifyOwnersRe = regexp.MustCompile(`(?mi)^/verify-owners\s*$`) ) @@ -324,7 +329,7 @@ func handle(ghc githubClient, gc git.ClientFactory, roc repoownersClient, log *l // prune old comments before adding a new one cp.PruneComments(func(comment github.IssueComment) bool { - return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, triggerConfig.JoinOrgURL)) + return strings.Contains(comment.Body, fmt.Sprintf(untrustedResponseFormat, ownersFileName, triggerConfig.JoinOrgURL, org)) }) if err := ghc.CreateComment(org, repo, number, markdownFriendlyComment(org, triggerConfig.JoinOrgURL, nonTrustedUsers)); err != nil { log.WithError(err).Errorf("Could not create comment for listing non-collaborators in %s files", ownersFileName) @@ -338,7 +343,7 @@ func handle(ghc githubClient, gc git.ClientFactory, roc repoownersClient, log *l return fmt.Errorf("failed removing %s label: %v", labels.InvalidOwners, err) } cp.PruneComments(func(comment github.IssueComment) bool { - return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, triggerConfig.JoinOrgURL)) + return strings.Contains(comment.Body, fmt.Sprintf(untrustedResponseFormat, ownersFileName, triggerConfig.JoinOrgURL, org)) }) } @@ -411,23 +416,24 @@ func parseOwnersFile(oc ownersClient, path string, c github.PullRequestChange, l return nil, owners } -func markdownFriendlyComment(org, joinOrgURL string, nonTrustedUsers map[string][]string) string { +func markdownFriendlyComment(org, joinOrgURL string, nonTrustedUsers map[string]nonTrustedReasons) string { var commentLines []string - commentLines = append(commentLines, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, joinOrgURL)) + commentLines = append(commentLines, fmt.Sprintf(untrustedResponseFormat, ownersFileName, joinOrgURL, org)) - for user, ownersFiles := range nonTrustedUsers { + for user, reasons := range nonTrustedUsers { commentLines = append(commentLines, fmt.Sprintf("- %s", user)) - for _, filename := range ownersFiles { + commentLines = append(commentLines, fmt.Sprintf(" - %s", reasons.triggerReason)) + for _, filename := range reasons.files { commentLines = append(commentLines, fmt.Sprintf(" - %s", filename)) } } return strings.Join(commentLines, "\n") } -func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, dir, patch string, ownerAliasesModified, skipTrustedUserCheck bool) (map[string][]string, sets.String, repoowners.RepoAliases, error) { +func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, dir, patch string, ownerAliasesModified, skipTrustedUserCheck bool) (map[string]nonTrustedReasons, sets.String, repoowners.RepoAliases, error) { repoAliases := make(repoowners.RepoAliases) - // nonTrustedUsers is a map of non-trusted users to the list of files they are being added in - nonTrustedUsers := map[string][]string{} + // nonTrustedUsers is a map of non-trusted users to the reasons they were not trusted + nonTrustedUsers := map[string]nonTrustedReasons{} trustedUsers := sets.String{} var err error @@ -458,7 +464,7 @@ func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, trigger return nonTrustedUsers, trustedUsers, repoAliases, nil } -func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, patch, fileName string, owners []string, nonTrustedUsers map[string][]string, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string][]string, error) { +func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, patch, fileName string, owners []string, nonTrustedUsers map[string]nonTrustedReasons, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string]nonTrustedReasons, error) { var err error for _, owner := range owners { // ignore if owner is an alias @@ -476,7 +482,8 @@ func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig // checkIfTrustedUser looks for newly addded owners by checking if they are in the patch // and then checks if the owner is a trusted user. -func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, owner, patch, fileName, org, repo string, nonTrustedUsers map[string][]string, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string][]string, error) { +// returns a map from user to reasons for not being trusted +func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, owner, patch, fileName, org, repo string, nonTrustedUsers map[string]nonTrustedReasons, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string]nonTrustedReasons, error) { // cap the number of checks to avoid exhausting tokens in case of large OWNERS refactors. if len(nonTrustedUsers)+trustedUsers.Len() > 50 { return nonTrustedUsers, nil @@ -488,33 +495,42 @@ func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugi } // if we already flagged the owner for the current file, return early - if ownersFiles, ok := nonTrustedUsers[owner]; ok { - for _, file := range ownersFiles { + if reasons, ok := nonTrustedUsers[owner]; ok { + for _, file := range reasons.files { if file == fileName { return nonTrustedUsers, nil } } - nonTrustedUsers[owner] = append(ownersFiles, fileName) + // have to separate assignment from map update due to map implementation (see "index expressions") + reasons.files = append(reasons.files, fileName) + nonTrustedUsers[owner] = reasons return nonTrustedUsers, nil } - isTrustedUser := trustedUsers.Has(owner) - if !isTrustedUser { - var err error - isTrustedUser, err = trigger.TrustedUser(ghc, triggerConfig.OnlyOrgMembers, triggerConfig.TrustedOrg, owner, org, repo) + isAlreadyTrusted := trustedUsers.Has(owner) + var err error + var triggerTrustedResponse trigger.TrustedUserResponse + if !isAlreadyTrusted { + triggerTrustedResponse, err = trigger.TrustedUser(ghc, triggerConfig.OnlyOrgMembers, triggerConfig.TrustedOrg, owner, org, repo) if err != nil { return nonTrustedUsers, err } } - if isTrustedUser { + if !isAlreadyTrusted && triggerTrustedResponse.IsTrusted { trustedUsers.Insert(owner) - } else { - if ownersFiles, ok := nonTrustedUsers[owner]; ok { - nonTrustedUsers[owner] = append(ownersFiles, fileName) + } else if !isAlreadyTrusted && !triggerTrustedResponse.IsTrusted { + if reasons, ok := nonTrustedUsers[owner]; ok { + reasons.triggerReason = triggerTrustedResponse.Reason + nonTrustedUsers[owner] = reasons } else { - nonTrustedUsers[owner] = []string{fileName} + nonTrustedUsers[owner] = nonTrustedReasons{ + // ensure that files is initialized to avoid nil pointer + files: []string{}, + triggerReason: triggerTrustedResponse.Reason, + } } } + return nonTrustedUsers, nil } diff --git a/prow/plugins/verify-owners/verify-owners_test.go b/prow/plugins/verify-owners/verify-owners_test.go index e0e10834434b..999cceffeed1 100644 --- a/prow/plugins/verify-owners/verify-owners_test.go +++ b/prow/plugins/verify-owners/verify-owners_test.go @@ -837,6 +837,7 @@ func TestNonCollaboratorsV2(t *testing.T) { } func testNonCollaborators(clients localgit.Clients, t *testing.T) { + const nonTrustedNotMemberNotCollaborator = "User is not a member of the org. User is not a collaborator." var tests = []struct { name string filesChanged []string @@ -848,6 +849,7 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { skipTrustedUserCheck bool shouldLabel bool shouldComment bool + commentShouldContain string }{ { name: "collaborators additions in OWNERS file", @@ -866,12 +868,13 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { shouldComment: false, }, { - name: "non-collaborators additions in OWNERS file", - filesChanged: []string{"OWNERS"}, - ownersFile: "nonCollaborators", - ownersPatch: "nonCollaboratorAdditions", - shouldLabel: true, - shouldComment: true, + name: "non-collaborators additions in OWNERS file", + filesChanged: []string{"OWNERS"}, + ownersFile: "nonCollaborators", + ownersPatch: "nonCollaboratorAdditions", + shouldLabel: true, + shouldComment: true, + commentShouldContain: nonTrustedNotMemberNotCollaborator, }, { name: "non-collaborators removal in OWNERS file", @@ -891,13 +894,14 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { shouldComment: false, }, { - name: "non-collaborators additions in OWNERS_ALIASES file", - filesChanged: []string{"OWNERS_ALIASES"}, - ownersFile: "collaboratorsWithAliases", - ownersAliasesFile: "nonCollaborators", - ownersAliasesPatch: "nonCollaboratorAdditions", - shouldLabel: true, - shouldComment: true, + name: "non-collaborators additions in OWNERS_ALIASES file", + filesChanged: []string{"OWNERS_ALIASES"}, + ownersFile: "collaboratorsWithAliases", + ownersAliasesFile: "nonCollaborators", + ownersAliasesPatch: "nonCollaboratorAdditions", + shouldLabel: true, + shouldComment: true, + commentShouldContain: nonTrustedNotMemberNotCollaborator, }, { name: "non-collaborators removals in OWNERS_ALIASES file", @@ -937,14 +941,15 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { shouldComment: false, }, { - name: "non-collaborators additions in both OWNERS and OWNERS_ALIASES file", - filesChanged: []string{"OWNERS", "OWNERS_ALIASES"}, - ownersFile: "nonCollaboratorsWithAliases", - ownersPatch: "nonCollaboratorsWithAliases", - ownersAliasesFile: "nonCollaborators", - ownersAliasesPatch: "nonCollaboratorAdditions", - shouldLabel: true, - shouldComment: true, + name: "non-collaborators additions in both OWNERS and OWNERS_ALIASES file", + filesChanged: []string{"OWNERS", "OWNERS_ALIASES"}, + ownersFile: "nonCollaboratorsWithAliases", + ownersPatch: "nonCollaboratorsWithAliases", + ownersAliasesFile: "nonCollaborators", + ownersAliasesPatch: "nonCollaboratorAdditions", + shouldLabel: true, + shouldComment: true, + commentShouldContain: nonTrustedNotMemberNotCollaborator, }, { name: "collaborator additions in both OWNERS and OWNERS_ALIASES file", @@ -965,21 +970,23 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { shouldComment: false, }, { - name: "non-collaborators additions in OWNERS file in vendor subdir, but include it", - filesChanged: []string{"vendor/k8s.io/client-go/OWNERS"}, - ownersFile: "nonCollaborators", - ownersPatch: "nonCollaboratorAdditions", - includeVendorOwners: true, - shouldLabel: true, - shouldComment: true, + name: "non-collaborators additions in OWNERS file in vendor subdir, but include it", + filesChanged: []string{"vendor/k8s.io/client-go/OWNERS"}, + ownersFile: "nonCollaborators", + ownersPatch: "nonCollaboratorAdditions", + includeVendorOwners: true, + shouldLabel: true, + shouldComment: true, + commentShouldContain: nonTrustedNotMemberNotCollaborator, }, { - name: "non-collaborators additions in OWNERS file in vendor dir", - filesChanged: []string{"vendor/OWNERS"}, - ownersFile: "nonCollaborators", - ownersPatch: "nonCollaboratorAdditions", - shouldLabel: true, - shouldComment: true, + name: "non-collaborators additions in OWNERS file in vendor dir", + filesChanged: []string{"vendor/OWNERS"}, + ownersFile: "nonCollaborators", + ownersPatch: "nonCollaboratorAdditions", + shouldLabel: true, + shouldComment: true, + commentShouldContain: nonTrustedNotMemberNotCollaborator, }, } lg, c, err := clients() @@ -1086,6 +1093,9 @@ func testNonCollaborators(clients localgit.Clients, t *testing.T) { if test.shouldComment && len(fghc.IssueComments[pr]) == 0 { t.Errorf("%s: expected comment but didn't receive", test.name) } + if test.shouldComment && len(test.commentShouldContain) > 0 && !strings.Contains(fghc.IssueComments[pr][0].Body, test.commentShouldContain) { + t.Errorf("%s: expected comment to contain\n%s\nbut it was actually\n%s", test.name, test.commentShouldContain, fghc.IssueComments[pr][0].Body) + } } } diff --git a/prow/plugins/welcome/welcome.go b/prow/plugins/welcome/welcome.go index 0360e0e06168..f2a04eb172eb 100644 --- a/prow/plugins/welcome/welcome.go +++ b/prow/plugins/welcome/welcome.go @@ -103,11 +103,11 @@ func handlePR(c client, t plugins.Trigger, pre github.PullRequestEvent, welcomeT repo := pre.PullRequest.Base.Repo.Name user := pre.PullRequest.User.Login - trusted, err := trigger.TrustedUser(c.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo) + trustedResponse, err := trigger.TrustedUser(c.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo) if err != nil { return fmt.Errorf("check if user %s is trusted: %v", user, err) } - if trusted { + if trustedResponse.IsTrusted { return nil }