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

prow/plugin: Extends label plugin to use Unique Prefixes #26383

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion prow/plugins/config.go
Expand Up @@ -391,6 +391,9 @@ type Label struct {
// defines to which repos this applies and can be `*` for global, an org
// or a repo in org/repo notation.
RestrictedLabels map[string][]RestrictedLabel `json:"restricted_labels,omitempty"`
// UniquePrefixes is the list of label prefixes that can only exist
// once per issue,
UniquePrefixes []string `json:"unique_prefixes,omitempty"`
}

func (l Label) RestrictedLabelsFor(org, repo string) map[string]RestrictedLabel {
Expand Down Expand Up @@ -1077,6 +1080,15 @@ func (c *Configuration) ValidatePluginsUnknown() error {
return utilerrors.NewAggregate(errors)
}

func validateUniquePrefixes(uniquePrefixes []string) error {
for _, prefix := range uniquePrefixes {
if len(prefix) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also look for duplicates, and perhaps also for one prefix being a prefix of another (foo and foobar eg) to avoid interaction between different prefixes

return fmt.Errorf("prefix cannot be an empty string")
}
}
return nil
}

func validateSizes(size Size) error {
if size.S > size.M || size.M > size.L || size.L > size.Xl || size.Xl > size.Xxl {
return errors.New("invalid size plugin configuration - one of the smaller sizes is bigger than a larger one")
Expand Down Expand Up @@ -1344,8 +1356,13 @@ func (c *Configuration) Validate() error {
if err := validateTrigger(c.Triggers); err != nil {
return err
}

validateRepoMilestone(c.RepoMilestone)

if err := validateUniquePrefixes(c.Label.UniquePrefixes); err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -2043,7 +2060,7 @@ func getLabelConfigFromRestrictedLabelsSlice(s []RestrictedLabel, label string)
func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos sets.String) {
equals := reflect.DeepEqual(c,
&Configuration{Approve: c.Approve, Bugzilla: c.Bugzilla, ExternalPlugins: c.ExternalPlugins,
Label: Label{RestrictedLabels: c.Label.RestrictedLabels}, Lgtm: c.Lgtm, Plugins: c.Plugins,
Label: Label{RestrictedLabels: c.Label.RestrictedLabels, UniquePrefixes: c.Label.UniquePrefixes}, Lgtm: c.Lgtm, Plugins: c.Plugins,
Triggers: c.Triggers, Welcome: c.Welcome})

if !equals || c.Bugzilla.Default != nil {
Expand Down
29 changes: 29 additions & 0 deletions prow/plugins/config_test.go
Expand Up @@ -104,6 +104,35 @@ func TestValidateExternalPlugins(t *testing.T) {
}
}

func TestValidateUniquePrefixes(t *testing.T) {
testCases := []struct {
name string
uniquePrefixes []string
isErrorNil bool
}{
{
name: "non-zero length prefixes",
uniquePrefixes: []string{"prefix1/", "prefix2"},
isErrorNil: true,
},
{
name: "zero length prefixes",
uniquePrefixes: []string{"", "prefix2"},
isErrorNil: false,
},
}
for _, tC := range testCases {
t.Run(tC.name, func(t *testing.T) {
err := validateUniquePrefixes(tC.uniquePrefixes)
if tC.isErrorNil && err != nil {
t.Errorf("expected nil error got %s", err)
} else if !tC.isErrorNil && err == nil {
t.Errorf("expected non nil error, got nil")
}
Comment on lines +127 to +131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we test for full error like the test above?

if !reflect.DeepEqual(err, test.expectedErr) {
t.Errorf("unexpected error: %v, expected: %v", err, test.expectedErr)
}

})
}
}

func TestSetDefault_Maps(t *testing.T) {
cases := []struct {
name string
Expand Down
37 changes: 34 additions & 3 deletions prow/plugins/label/label.go
Expand Up @@ -49,6 +49,20 @@ func init() {
plugins.RegisterPullRequestHandler(PluginName, handlePullRequest, helpProvider)
}

// getExistingUniquePrefixLabels returns a list of labels for every label prefix that can only exist once per issue
func getExistingUniquePrefixLabels(config plugins.Label, labels []github.Label) map[string][]string {
labelsMap := make(map[string][]string)
prefixList := config.UniquePrefixes
for _, prefix := range prefixList {
for _, label := range labels {
if matched := strings.HasPrefix(label.Name, prefix); matched {
labelsMap[prefix] = append(labelsMap[prefix], label.Name)
}
}
}
return labelsMap
}

func configString(labels []string) string {
var formattedLabels []string
for _, label := range labels {
Expand Down Expand Up @@ -168,7 +182,6 @@ func handleComment(gc githubClient, log *logrus.Entry, config plugins.Label, e *
if err != nil {
return err
}

RepoLabelsExisting := sets.String{}
for _, l := range repoLabels {
RepoLabelsExisting.Insert(strings.ToLower(l.Name))
Expand All @@ -180,6 +193,7 @@ func handleComment(gc githubClient, log *logrus.Entry, config plugins.Label, e *
labelsToAdd []string
labelsToRemove []string
nonMemberTriageAccepted bool
labelPrefix string
)

additionalLabelSet := sets.String{}
Expand Down Expand Up @@ -228,12 +242,29 @@ func handleComment(gc githubClient, log *logrus.Entry, config plugins.Label, e *
gc.CreateComment(org, repo, e.Number, plugins.FormatResponseRaw(bodyWithoutComments, e.HTMLURL, e.User.Login, canNotSetLabelReason))
continue
}

uniqueLabelsMap := getExistingUniquePrefixLabels(config, labels)
var labelsWithPrefixExist = false
for prefix := range uniqueLabelsMap {
if matched := strings.HasPrefix(labelToAdd, prefix); matched {
labelPrefix = prefix
labelsWithPrefixExist = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need an additional variable, we can just reset labelPrefix in each iteration and use labelPrefix != "" as a check

}
}
// if the labelToAdd matches a unique prefix, remove all pre existing
// labels that match this unique prefix, and then add the new label
if labelsWithPrefixExist {
for _, label := range uniqueLabelsMap[labelPrefix] {
if label != labelToAdd && github.HasLabel(label, labels) {
if err := gc.RemoveLabel(org, repo, e.Number, label); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ability to remote a labelj can be behind a permission (see L279), so we need to test whether the user is able to remove the label (and handle that situation well, as ignoring that condition would lead to a duplicate label on the issue/pr)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @petr-muller thanks for reviewing this!
I wanted some clarification on this comment.

L235 already has the check which checks user permissions (which when fails, does not enter the de-duplication logic that I have added, and you have pointed to).

I'm not sure how this can lead to duplicate labels?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L235 checks for the permission to add the new label, but your L258 removes a different label, which the user may not have a permission to remove. Imagine this:

  • Prow is configured to restrict priority/urgent to senior engineers
  • Prow is configured to only allow one label with priority/ prefix
  • PR has priority/urgent
  • Non-senior engineer labels the PR with priority/low

Your code would remove the priority/urgent (which the user has no permission to remove normally). If you fix this simply by denying the removal but allow adding priority/low then you have duplicate labels.

log.WithError(err).Errorf("GitHub failed to remove the following label: %s", label)
}
}
}
}
if err := gc.AddLabel(org, repo, e.Number, labelToAdd); err != nil {
log.WithError(err).WithField("label", labelToAdd).Error("GitHub failed to add the label")
}
}

// Remove labels
for _, labelToRemove := range labelsToRemove {
if !github.HasLabel(labelToRemove, labels) {
Expand Down
15 changes: 14 additions & 1 deletion prow/plugins/label/label_test.go
Expand Up @@ -64,6 +64,7 @@ func TestHandleComment(t *testing.T) {
expectedCommentText string
action github.GenericCommentEventAction
teams map[string]map[string]fakegithub.TeamWithMembers
uniquePrefixes []string
}
testcases := []testCase{
{
Expand Down Expand Up @@ -475,6 +476,17 @@ func TestHandleComment(t *testing.T) {
commenter: orgMember,
action: github.GenericCommentActionCreated,
},
{
name: "Add label that has a prefix that can only exist once per issue",
body: "/priority urgent",
repoLabels: []string{"area/go", "priority/urgent", "priority/critical", "priority/l"},
issueLabels: []string{"area/go", "priority/critical", "priority/l"},
expectedNewLabels: formatWithPRInfo("priority/urgent"),
expectedRemovedLabels: formatWithPRInfo("priority/critical", "priority/l"),
commenter: orgMember,
action: github.GenericCommentActionCreated,
uniquePrefixes: []string{"priority/"},
},
{
name: "Do nothing with empty /label command",
body: "/label",
Expand Down Expand Up @@ -762,7 +774,8 @@ func TestHandleComment(t *testing.T) {
Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"},
User: github.User{Login: tc.commenter},
}
err := handleComment(fakeClient, logrus.WithField("plugin", PluginName), plugins.Label{AdditionalLabels: tc.extraLabels, RestrictedLabels: tc.restrictedLabels}, e)
err := handleComment(fakeClient, logrus.WithField("plugin", PluginName), plugins.Label{AdditionalLabels: tc.extraLabels, RestrictedLabels: tc.restrictedLabels, UniquePrefixes: tc.uniquePrefixes}, e)

if err != nil {
t.Fatalf("didn't expect error from handle comment test: %v", err)
}
Expand Down
5 changes: 5 additions & 0 deletions prow/plugins/plugin-config-documented.yaml
Expand Up @@ -487,6 +487,11 @@ label:
# or a repo in org/repo notation.
restricted_labels:
"": null

# UniquePrefixes is the list of label prefixes that can only exist
# once per issue.
unique_prefixes:
- ""
lgtm:
- # Repos is either of the form org/repos or just org.
repos:
Expand Down