Expose maximum assignee limit #10554

kanishkarj · 2018-12-25T21:51:33Z

Fix to the issue #10391.

k8s-ci-robot · 2018-12-25T21:51:34Z

Welcome @kanishkarj! It looks like this is your first PR to kubernetes/test-infra 🎉🎉

k8s-ci-robot · 2018-12-25T21:51:35Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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-ci-robot · 2018-12-25T21:51:40Z

Hi @kanishkarj. 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.

kanishkarj · 2018-12-25T21:56:32Z

Signed.

ibrasho · 2018-12-26T00:28:01Z

Welcome. 🎉

I think the issue is that Github API silently fails and doesn't return an error when attempting to assign more than 10 people to the issue/PR, so we can't tell if the failure was because the user is not an org member or becuase we exceeded the limit.

We should add some explanation to the current error message instead of replacing it as it's still valid.

ibrasho · 2018-12-26T00:30:46Z

prow/plugins/assign/assign.go

 	addFailureResponse := func(mu github.MissingUsers) string {
-		return fmt.Sprintf("GitHub didn't allow me to assign the following users: %s.\n\nNote that only [%s members](https://github.com/orgs/%s/people) and repo collaborators can be assigned. \nFor more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/#issue-assignment-in-github)", strings.Join(mu.Users, ", "), org, org)
+		return "Note that only a maximum of 10 members and repo collaborators can be assigned"


I'd suggest the following:

return fmt.Sprintf("GitHub didn't allow me to assign the following users: %s.\n\nNote that only [%s members](https://github.com/orgs/%s/people) and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.\nFor more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/#issue-assignment-in-github)", strings.Join(mu.Users, ", "), org, org)

Okay, I shall put that up.

kanishkarj · 2018-12-26T03:43:43Z

@ibrasho yes I do agree with it. But I am not able to understand why it is failing, so I don't know any other alternative. Any suggestions on how to go about it?

kanishkarj · 2018-12-26T08:33:22Z

@ibrasho I have made another commit, check it out!

nikhita · 2018-12-26T15:11:23Z

But I am not able to understand why it is failing, so I don't know any other alternative. Any suggestions on how to go about it?

@kanishkarj Added a comment here: #10391 (comment).

nikhita · 2018-12-26T15:15:17Z

@kanishkarj Can you also update the PR title to be more descriptive of the change? Something like prow/assign: update error for more than 10 assignees.

So that it's easier for people to understand at a first glance what the PR does. :)

stevekuznetsov

Nice!

stevekuznetsov · 2018-12-26T15:55:57Z

prow/plugins/assign/assign_test.go

+			assigned:  []string{"user1", "user2", "user3", "user4", "user5", "user6", "user7", "user8", "user9", "user10", "user11"},
+		},
+		{
+			name:       "assign >10 users",


This name seems incorrect

stevekuznetsov · 2018-12-26T15:56:14Z

prow/plugins/assign/assign.go

@@ -188,7 +188,7 @@ type handler struct {
 func newAssignHandler(e github.GenericCommentEvent, gc githubClient, log *logrus.Entry) *handler {
 	org := e.Repo.Owner.Login
 	addFailureResponse := func(mu github.MissingUsers) string {
-		return fmt.Sprintf("GitHub didn't allow me to assign the following users: %s.\n\nNote that only [%s members](https://github.com/orgs/%s/people) and repo collaborators can be assigned. \nFor more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/#issue-assignment-in-github)", strings.Join(mu.Users, ", "), org, org)
+		return fmt.Sprintf("GitHub didn't allow me to assign the following users: %s.\n\nNote that only [%s members](https://github.com/orgs/%s/people) and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.\nFor more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/#issue-assignment-in-github)", strings.Join(mu.Users, ", "), org, org)


Maybe

Note that only ten or fewer [%s members...

?

(this is a nit-pick)

Can't we also detect the >10 case here and make the error message specific to the error that happened?

Assigning 11+ people at the same time seems like an edge case. How do you feel about only mentioning this fact if a) more than 10 people were attempted to be assigned and b) github returns an error?

Actually given @ibrasho comments above that isn't going to work.

stevekuznetsov · 2018-12-26T15:56:27Z

/ok-to-test

kanishkarj · 2018-12-26T22:46:02Z

I have updated the tests, go through them, please.

kanishkarj · 2018-12-27T19:47:04Z

It can be merged right?

fejta

Thanks for adding tests
/lgtm

k8s-ci-robot · 2019-01-07T18:14:30Z

LGTM label has been added.

Git tree hash: d6cc338b297b24944a22d75a78190a67d0549351

k8s-ci-robot · 2019-01-07T18:14:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, kanishkarj

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [fejta]

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

cjwagner · 2019-01-08T17:37:10Z

@kanishkarj FYI we generally avoid merge commits in PR branches to make the git history easier to understand.

kanishkarj added 2 commits December 26, 2018 03:16

Fix issue kubernetes#10391

5cda10f

Merge branch 'master' of https://github.com/kubernetes/test-infra

dd367a1

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 25, 2018

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 25, 2018

k8s-ci-robot requested review from cjwagner and fejta December 25, 2018 21:51

k8s-ci-robot added the area/prow Issues or PRs related to prow label Dec 25, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 25, 2018

ibrasho reviewed Dec 26, 2018

View reviewed changes

Update the Error Format

5befcec

stevekuznetsov reviewed Dec 26, 2018

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 Dec 26, 2018

stevekuznetsov changed the title ~~Fix for Issue #10391~~ Expose maximum assignee limit Dec 26, 2018

Update tests

26e6fbd

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 26, 2018

stronger test

3a9826b

k8s-ci-robot assigned fejta Jan 7, 2019

fejta approved these changes Jan 7, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2019

k8s-ci-robot merged commit f7ba412 into kubernetes:master Jan 7, 2019

ibrasho mentioned this pull request Jan 14, 2019

REQUEST: New membership for @ibrasho kubernetes/org#352

Closed

6 tasks

nikhita mentioned this pull request Jun 25, 2019

prow gives no user-facing indication of max 10 assignee limit via /assign #10391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose maximum assignee limit #10554

Expose maximum assignee limit #10554

kanishkarj commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

kanishkarj commented Dec 25, 2018

ibrasho commented Dec 26, 2018 •

edited

Loading

ibrasho Dec 26, 2018

kanishkarj Dec 26, 2018

kanishkarj commented Dec 26, 2018

kanishkarj commented Dec 26, 2018

nikhita commented Dec 26, 2018

nikhita commented Dec 26, 2018 •

edited

Loading

stevekuznetsov left a comment

stevekuznetsov Dec 26, 2018

stevekuznetsov Dec 26, 2018

stevekuznetsov Dec 26, 2018

stevekuznetsov Jan 2, 2019

fejta Jan 7, 2019

fejta Jan 7, 2019

stevekuznetsov commented Dec 26, 2018

kanishkarj commented Dec 26, 2018

kanishkarj commented Dec 27, 2018

fejta left a comment

k8s-ci-robot commented Jan 7, 2019

k8s-ci-robot commented Jan 7, 2019

cjwagner commented Jan 8, 2019

Expose maximum assignee limit #10554

Expose maximum assignee limit #10554

Conversation

kanishkarj commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

k8s-ci-robot commented Dec 25, 2018

kanishkarj commented Dec 25, 2018

ibrasho commented Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanishkarj commented Dec 26, 2018

kanishkarj commented Dec 26, 2018

nikhita commented Dec 26, 2018

nikhita commented Dec 26, 2018 • edited Loading

stevekuznetsov 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

stevekuznetsov commented Dec 26, 2018

kanishkarj commented Dec 26, 2018

kanishkarj commented Dec 27, 2018

fejta left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 7, 2019

k8s-ci-robot commented Jan 7, 2019

cjwagner commented Jan 8, 2019

ibrasho commented Dec 26, 2018 •

edited

Loading

nikhita commented Dec 26, 2018 •

edited

Loading