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

RaghavRoy145 · 2022-05-21T18:48:32Z

This commit takes care of label prefixes that can only exist once per issue. This is done by using a list of unique prefixes to create a map that maps these prefixes to existing labels that match the prefix. When a new label is needed to be added that matches a unique prefix, the pre-existing labels in this map that match the same prefix are removed and then the new label is added.

k8s-ci-robot · 2022-05-21T18:48:41Z

Hi @RaghavRoy145. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

RaghavRoy145 · 2022-05-21T18:48:53Z

/assign @nikhita

RaghavRoy145 · 2022-05-21T19:04:03Z

/cc @MadhavJivrajani

MadhavJivrajani

Spoke with @RaghavRoy145 offline, left a few minor suggestions below. @nikhita could you weigh in on the below points as I may be lacking some context here:

Is the intent of this change to be a mechanism that ensures unique prefixes or is the intent that a unique label that matches a given regex should exist?
- Because there might be cases where the user could provide non-prefix regexes
- And also there might be a few funny cases where a regex could match all labels and the implementation can remove all of them (providing safeguards probably isn't too difficult here tho).
If we go with the former, can we drop regexes altogether and stick with strings.HasPrefix?
- Similar concerns as above for HasPrefix, we would probably need a validate function for the Label plugin in config.go that ensures that no item in the UniquePrefixes list is an empty string (since "" is a prefix to everything).

MadhavJivrajani · 2022-06-10T08:27:51Z

prow/plugins/config.go

+	// UniquePrefixList is the list of label prefixes that can only exist
+	// once per issue,
+	UniquePrefixes []string `json:"unique_prefixes,omitempty"`


Nit: UniquePrefixes the field name is not the same as UniquePrefixList which is mentioned in the comments.

We should go with UniquePrefixes since it sounds more canonical and fix the comment.

the naming should be corrected in the test file as well.

MadhavJivrajani · 2022-06-10T08:28:39Z

prow/plugins/label/label.go

 Copyright 2016 The Kubernetes Authors.
-
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
 You may obtain a copy of the License at
-
    http://www.apache.org/licenses/LICENSE-2.0
-
 Unless required by applicable law or agreed to in writing, software


This change isn't needed :D

MadhavJivrajani · 2022-06-10T08:39:34Z

prow/plugins/label/label.go

+	prefixList := config.UniquePrefixes
+	for _, prefix := range prefixList {
+		for _, label := range labels {
+			if matched, _ := regexp.MatchString(prefix, label.Name); matched {


We probably shouldn't ignore errors here. Errors would creep up in case the regex can't be Compiled.

MadhavJivrajani · 2022-06-10T08:49:34Z

prow/plugins/label/label.go

+		// The second check (labelSplit[0] == "priority") can be removed or extended in the future to generalise
+		// the functionality of removing labels of the same class
+		if len(labelSplit) == 2 && labelSplit[0] == "priority" {
+			labelsMap[labelSplit[0]] = append(labelsMap[labelSplit[0]], labelSplit[1])


Is this logic still needed? If I understand correctly, this change is meant for unique prefixes and not necessarily just priority labels.

MadhavJivrajani · 2022-06-10T08:58:41Z

prow/plugins/label/label.go

+		uniqueLabelsMap := getExistingUniquePrefixlabels(config, labels)
+		var labelsWithPrefixExist = false
+		for prefix := range uniqueLabelsMap {
+			if matched, _ := regexp.MatchString(prefix, labelToAdd); matched {


Same comment as above regarding ignoring errors.

MadhavJivrajani · 2022-06-10T09:49:06Z

/ok-to-test

MadhavJivrajani · 2022-06-10T10:37:34Z

/retest
~~(might be an infra error, but to be sure - retesting)~~

nikhita · 2022-06-22T04:13:10Z

@MadhavJivrajani answering your questions from #26383 (review):

Is the intent of this change to be a mechanism that ensures unique prefixes or is the intent that a unique label that matches a given regex should exist?

Neither. The intent is to follow something similar to the lifecycle label:

test-infra/prow/plugins/lifecycle/lifecycle.go

Lines 129 to 138 in 0cd7957

    
           // If the label does not exist and we asked for it to be added, 
        
           // remove other existing lifecycle labels and add it. 
        
           if !github.HasLabel(lbl, labels) && !remove { 
        
           	for _, label := range lifecycleLabels { 
        
           		if label != lbl && github.HasLabel(label, labels) { 
        
           			if err := gc.RemoveLabel(org, repo, number, label); err != nil { 
        
           				log.WithError(err).Errorf("GitHub failed to remove the following label: %s", label) 
        
           			} 
        
           		} 
        
           	}

If a PR already has priority/important-longterm and I add priority/critical-urgent, then prow should remove the old label priority/important-longterm and add the priorirty/critical-urgent label.

If we go with the former, can we drop regexes altogether and stick with strings.HasPrefix?

I agree. Might be easier to deal with errors if we use strings.HasPrefix.

@RaghavRoy145 the PR body was empty. I have edited the PR body to point to the issue that this PR is resolving. Can you also add more content to the PR body with details on why/what this PR is doing? Please also include these details in the commit message.

MadhavJivrajani · 2022-06-22T04:22:37Z

Thanks Nikhita!

Neither. The intent is to follow something similar to the lifecycle label

That’s my bad, I should’ve been clearer. I agree with your point! I meant unique prefixes as a mechanism to decide if something with that prefix exists (for ex priority/) and if it does, remove it and add the one that’s commented (like you said).

This is probably for another PR, but if we do end up doing a generic mechanism to implement the above (I.e. not just restrict to priority or lifecycle labels), we could have the opportunity to maybe reuse this for the lifecycle label logic as well.

k8s-triage-robot · 2022-11-24T04:06:02Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

This commit takes care of label prefixes that can only exist once per issue. This is done by using a list of unique prefixes to create a map that maps these prefixes to existing labels that match the prefix. When a new label is needed to be added that matches a unique prefix, the pre-existing labels in this map that match the same prefix are removed and then the new label is added. Signed-off-by: Raghav Roy <raghavroy145@gmail.com>

k8s-ci-robot · 2022-12-19T13:44:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RaghavRoy145
Once this PR has been reviewed and has the lgtm label, please assign cjwagner for approval by writing /assign @cjwagner in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

prow/plugins/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RaghavRoy145 · 2022-12-19T14:09:51Z

Hey @MadhavJivrajani
Rebased to resolve test failures, can you take a look?
Thanks!

petr-muller · 2022-12-19T14:32:44Z

/remove-lifecycle stale

petr-muller · 2022-12-19T14:36:24Z

prow/plugins/config.go

@@ -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 {


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

petr-muller · 2022-12-19T14:40:50Z

prow/plugins/config_test.go

+			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")
+			}


Could we test for full error like the test above?

test-infra/prow/plugins/config_test.go

Lines 101 to 103 in 37b977f

if !reflect.DeepEqual(err, test.expectedErr) {

t.Errorf("unexpected error: %v, expected: %v", err, test.expectedErr)

}

petr-muller · 2022-12-19T14:46:41Z

prow/plugins/label/label.go

+		for prefix := range uniqueLabelsMap {
+			if matched := strings.HasPrefix(labelToAdd, prefix); matched {
+				labelPrefix = prefix
+				labelsWithPrefixExist = true


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

petr-muller · 2022-12-19T15:09:33Z

prow/plugins/label/label.go

+		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 {


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)

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?

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.

k8s-ci-robot · 2023-02-10T05:49:35Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-02-15T17:08:22Z

@RaghavRoy145: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-test-infra-verify-cri-o	`37b977f`	link	true	`/test pull-test-infra-verify-cri-o`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2023-07-04T18:20:40Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-01-19T02:00:46Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-02-18T02:52:12Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-02-18T02:52:18Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2022

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/prow Issues or PRs related to prow labels May 21, 2022

k8s-ci-robot requested review from alvaroaleman and michelle192837 May 21, 2022 18:48

k8s-ci-robot added area/prow/plugins Issues or PRs related to prow's plugins for the hook component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 21, 2022

k8s-ci-robot assigned nikhita May 21, 2022

k8s-ci-robot requested a review from MadhavJivrajani May 21, 2022 19:04

RaghavRoy145 force-pushed the allow-only-one-priority-label branch from 3c7dc01 to af391b8 Compare June 10, 2022 09:16

MadhavJivrajani reviewed Jun 10, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2022

RaghavRoy145 force-pushed the allow-only-one-priority-label branch from af391b8 to 0aa2a88 Compare June 10, 2022 11:01

RaghavRoy145 force-pushed the allow-only-one-priority-label branch from 0aa2a88 to 0ccef3b Compare June 24, 2022 10:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2022

RaghavRoy145 force-pushed the allow-only-one-priority-label branch from 0ccef3b to 471cd8e Compare June 24, 2022 10:47

RaghavRoy145 changed the title ~~[WIP] Fix for handling labels with unique prefixes~~ prow/plugin: Extends label plugin to use Unique Prefixes Jun 24, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 24, 2022

RaghavRoy145 force-pushed the allow-only-one-priority-label branch from 471cd8e to 37b977f Compare December 19, 2022 13:44

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2022

petr-muller reviewed Dec 19, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 4, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 19, 2024

k8s-ci-robot closed this Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RaghavRoy145 commented May 21, 2022 •

edited

k8s-ci-robot commented May 21, 2022

RaghavRoy145 commented May 21, 2022

RaghavRoy145 commented May 21, 2022

MadhavJivrajani left a comment

MadhavJivrajani Jun 10, 2022

palnabarun Jun 15, 2022

Namanl2001 Jun 15, 2022

MadhavJivrajani Jun 10, 2022

MadhavJivrajani Jun 10, 2022

MadhavJivrajani Jun 10, 2022

MadhavJivrajani Jun 10, 2022

MadhavJivrajani commented Jun 10, 2022

MadhavJivrajani commented Jun 10, 2022 •

edited

nikhita commented Jun 22, 2022

MadhavJivrajani commented Jun 22, 2022 •

edited

k8s-triage-robot commented Nov 24, 2022

k8s-ci-robot commented Dec 19, 2022

RaghavRoy145 commented Dec 19, 2022

petr-muller commented Dec 19, 2022

petr-muller Dec 19, 2022

petr-muller Dec 19, 2022

petr-muller Dec 19, 2022

petr-muller Dec 19, 2022

RaghavRoy145 Dec 19, 2022

petr-muller Dec 19, 2022

k8s-ci-robot commented Feb 10, 2023

k8s-ci-robot commented Feb 15, 2023

k8s-triage-robot commented Jul 4, 2023

k8s-triage-robot commented Jan 19, 2024

k8s-triage-robot commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

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

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

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

Conversation

RaghavRoy145 commented May 21, 2022 • edited

k8s-ci-robot commented May 21, 2022

RaghavRoy145 commented May 21, 2022

RaghavRoy145 commented May 21, 2022

MadhavJivrajani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadhavJivrajani commented Jun 10, 2022

MadhavJivrajani commented Jun 10, 2022 • edited

nikhita commented Jun 22, 2022

MadhavJivrajani commented Jun 22, 2022 • edited

k8s-triage-robot commented Nov 24, 2022

k8s-ci-robot commented Dec 19, 2022

RaghavRoy145 commented Dec 19, 2022

petr-muller commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 10, 2023

k8s-ci-robot commented Feb 15, 2023

k8s-triage-robot commented Jul 4, 2023

k8s-triage-robot commented Jan 19, 2024

k8s-triage-robot commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

RaghavRoy145 commented May 21, 2022 •

edited

MadhavJivrajani commented Jun 10, 2022 •

edited

MadhavJivrajani commented Jun 22, 2022 •

edited