Report problem when label added as both restricted and additional

Signed-off-by: Jakub Guzik <[email protected]>
kubernetes · Nov 5, 2021 · e6b6393 · e6b6393
1 parent b80d51f
commit e6b6393
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 16 deletions.
diff --git a/prow/cmd/checkconfig/main.go b/prow/cmd/checkconfig/main.go
@@ -426,8 +426,8 @@ func validate(o options) error {
 	}
 
 	if pcfg != nil && o.warningEnabled(validateLabelWarning) {
-		if err := verifyLabelPlugin(pcfg.Label); err != nil {
-			errs = append(errs, err)
+		if errList := verifyLabelPlugin(pcfg.Label); len(errList) > 0 {
+			errs = append(errs, errList...)
 		}
 	}
 
@@ -1052,23 +1052,44 @@ func verifyOwnersPlugin(cfg *plugins.Configuration) error {
 	return nil
 }
 
-func verifyLabelPlugin(label plugins.Label) error {
+func checkRestrictedLabelNotInAdditionalLables(restricted string, label plugins.Label) bool {
+	for _, additional := range label.AdditionalLabels {
+		if restricted == additional {
+			return true
+		}
+	}
+	return false
+}
+
+func verifyLabelPlugin(label plugins.Label) []error {
 	var orgRepos []string
+	var errs []error
+	misconfiguredLabels := make(map[string][]string)
 	for orgRepo, restrictedLabels := range label.RestrictedLabels {
 		for _, restrictedLabel := range restrictedLabels {
+			if found := checkRestrictedLabelNotInAdditionalLables(restrictedLabel.Label, label); found {
+				misconfiguredLabels[restrictedLabel.Label] = append(misconfiguredLabels[restrictedLabel.Label], orgRepo)
+			}
 			if restrictedLabel.Label == "" {
 				orgRepos = append(orgRepos, orgRepo)
 			}
 		}
 	}
 
+	if len(misconfiguredLabels) > 0 {
+		for label, repos := range misconfiguredLabels {
+			errs = append(errs,
+				fmt.Errorf("the following orgs or repos have configuration of label plugin using the restricted label %s which is also configured as an additional label: %s", label, strings.Join(repos, ", ")))
+		}
+	}
+
 	if len(orgRepos) > 0 {
 		sort.Strings(orgRepos)
-		return fmt.Errorf("the following orgs or repos have configuration of %s plugin using the empty string as label name in restricted labels: %s",
+		errs = append(errs, fmt.Errorf("the following orgs or repos have configuration of %s plugin using the empty string as label name in restricted labels: %s",
 			labelplugin.PluginName, strings.Join(orgRepos, ", "),
-		)
+		))
 	}
-	return nil
+	return errs
 }
 
 func validateTriggers(cfg *config.Config, pcfg *plugins.Configuration) error {

diff --git a/prow/cmd/checkconfig/main_test.go b/prow/cmd/checkconfig/main_test.go
@@ -2223,9 +2223,9 @@ func TestValidateGitHubAppIsInstalled(t *testing.T) {
 func TestVerifyLabelPlugin(t *testing.T) {
 	t.Parallel()
 	testCases := []struct {
-		name             string
-		label            plugins.Label
-		expectedErrorMsg string
+		name              string
+		label             plugins.Label
+		expectedErrorMsgs []string
 	}{
 		{
 			name: "empty label config is valid",
@@ -2246,7 +2246,7 @@ func TestVerifyLabelPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErrorMsg: "the following orgs or repos have configuration of label plugin using the empty string as label name in restricted labels: openshift/machine-config-operator",
+			expectedErrorMsgs: []string{"the following orgs or repos have configuration of label plugin using the empty string as label name in restricted labels: openshift/machine-config-operator"},
 		},
 		{
 			name: "valid after removing the restricted labels for the empty string",
@@ -2279,18 +2279,72 @@ func TestVerifyLabelPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErrorMsg: "the following orgs or repos have configuration of label plugin using the empty string as label name in restricted labels: orgRepo1, orgRepo2",
+			expectedErrorMsgs: []string{"the following orgs or repos have configuration of label plugin using the empty string as label name in restricted labels: orgRepo1, orgRepo2"},
+		},
+		{
+			name: "invalid when additional and restricted labels are the same",
+			label: plugins.Label{
+				AdditionalLabels: []string{"cherry-pick-approved"},
+				RestrictedLabels: map[string][]plugins.RestrictedLabel{
+					"orgRepo1": {
+						{
+							Label:        "cherry-pick-approved",
+							AllowedTeams: []string{"some-other-team"},
+						},
+					},
+				},
+			},
+			expectedErrorMsgs: []string{"the following orgs or repos have configuration of label plugin using the restricted label cherry-pick-approved which is also configured as an additional label: orgRepo1"},
+		},
+		{
+			name: "invalid when additional and restricted labels are the same in multiple orgRepos and empty string",
+			label: plugins.Label{
+				AdditionalLabels: []string{"cherry-pick-approved", "cherry-pick-not-approved"},
+				RestrictedLabels: map[string][]plugins.RestrictedLabel{
+					"orgRepo1": {
+						{
+							Label:        "cherry-pick-approved",
+							AllowedTeams: []string{"some-other-team"},
+						},
+					},
+					"orgRepo2": {
+						{
+							Label:        "",
+							AllowedUsers: []string{"some-user"},
+						},
+					},
+					"orgRepo3": {
+						{
+							Label:        "cherry-pick-approved",
+							AllowedUsers: []string{"some-user"},
+						},
+					},
+					"orgRepo4": {
+						{
+							Label:        "cherry-pick-not-approved",
+							AllowedUsers: []string{"some-user"},
+						},
+					},
+				},
+			},
+			expectedErrorMsgs: []string{
+				"the following orgs or repos have configuration of label plugin using the restricted label cherry-pick-approved which is also configured as an additional label: orgRepo1, orgRepo3",
+				"the following orgs or repos have configuration of label plugin using the restricted label cherry-pick-not-approved which is also configured as an additional label: orgRepo4",
+				"the following orgs or repos have configuration of label plugin using the empty string as label name in restricted labels: orgRepo2",
+			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			var actualErrMsg string
-			if err := verifyLabelPlugin(tc.label); err != nil {
-				actualErrMsg = err.Error()
+			var actualErrMsgs []string
+			if errs := verifyLabelPlugin(tc.label); len(errs) != 0 {
+				for _, err := range errs {
+					actualErrMsgs = append(actualErrMsgs, err.Error())
+				}
 			}
-			if actualErrMsg != tc.expectedErrorMsg {
-				t.Errorf("expected error %q, got error %q", tc.expectedErrorMsg, actualErrMsg)
+			if !reflect.DeepEqual(actualErrMsgs, tc.expectedErrorMsgs) {
+				t.Errorf("expected error %q, got error %q", tc.expectedErrorMsgs, actualErrMsgs)
 			}
 		})
 	}