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

Update command help for assign plugin #13292

Conversation

@RainbowMango
Copy link
Contributor

commented Jul 2, 2019

It's a new feature by GitHub such that it allows to assign anyone who has commented on the issue/PR, even if they are not a member.

https://github.blog/changelog/2019-06-25-assign-issues-to-issue-commenters/

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Welcome @RainbowMango!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

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

@matthyx

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

/ok-to-test
@stevekuznetsov is that really what we want, or should we somehow block this new feature?

@RainbowMango

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

/assign @nikhita

@nikhita

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

The error message would also need to be updated:

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)

@nikhita

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

is that really what we want, or should we somehow block this new feature?

Personally, I see this as useful so that we can assign issues to new/new-ish contributors. I think we can tighten if we see cases of abuse regarding this. We follow the allow-until-abused approach for a bunch of things today (closing issues, etc). 😬

@nikhita

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

/cc @cblecker

@k8s-ci-robot k8s-ci-robot requested a review from cblecker Jul 2, 2019

@RainbowMango RainbowMango force-pushed the RainbowMango:pr_update_command_help_for_assign branch from 9eb73e8 to 29b91a6 Jul 2, 2019

@RainbowMango

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

The error message would also need to be updated:

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)

@nikhita Please help take a look the updated error message.

BTW, the contributor guide need to be updated too, i will update it later.

@@ -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 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)
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 the people should have commented on the issues/PRs 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)

This comment has been minimized.

Copy link
@nikhita

nikhita Jul 2, 2019

Member

I think we should preserve repo collaborators because they can be assigned as well.

So maybe something like members, repo collaborators and people who have commented on this issue/PR can be assigned...?

This comment has been minimized.

Copy link
@RainbowMango

RainbowMango Jul 2, 2019

Author Contributor

It's fine to me.

Just a question. Are there people who is the collaborator but not a member?

This comment has been minimized.

Copy link
@nikhita

nikhita Jul 2, 2019

Member

Are there people who is the collaborator but not a member?

Collaborators who are not members are outside collaborators. We don't want to have them in upstream Kubernetes, but I believe we still have some outside collaborators (we intend to fix this).

Other users of prow can have cases where they have outside collaborators so it's useful to keep this line in. :)

This comment has been minimized.

Copy link
@RainbowMango

RainbowMango Jul 2, 2019

Author Contributor

OK. Thanks for your patience and quick response. :)

@RainbowMango RainbowMango force-pushed the RainbowMango:pr_update_command_help_for_assign branch from 29b91a6 to 75bbdcc Jul 2, 2019

@@ -49,7 +49,7 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin
Usage: "/[un]assign [[@]<username>...]",
Description: "Assigns an assignee to the PR",
Featured: true,
WhoCanUse: "Anyone can use the command, but the target user must be a member of the org that owns the repository.",
WhoCanUse: "Anyone can use the command, but the target user must be a member of the org that owns the repository or should have commented on the issue or PR.",

This comment has been minimized.

Copy link
@cblecker

cblecker Jul 2, 2019

Member
Suggested change
WhoCanUse: "Anyone can use the command, but the target user must be a member of the org that owns the repository or should have commented on the issue or PR.",
WhoCanUse: "Anyone can use the command, but the target user must be an org member, a repo collaborator, or should have previously commented on the issue or PR.",

This comment has been minimized.

Copy link
@RainbowMango

RainbowMango Jul 3, 2019

Author Contributor

Thanks. Much better now.

This comment has been minimized.

Copy link
@RainbowMango

RainbowMango Jul 3, 2019

Author Contributor

@nikhita Any comments?

Maybe we should add yourself also, like follows:
but the target user must be an org member, a repo collaborator, yourself, or should have previously commented on the issue or PR.

@@ -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 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)
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), repo collaborators and people who have commented on this issue/PR 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)

This comment has been minimized.

Copy link
@cblecker

cblecker Jul 2, 2019

Member
Suggested change
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), repo collaborators and people who have commented on this issue/PR 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)
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), repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, 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)

@RainbowMango RainbowMango force-pushed the RainbowMango:pr_update_command_help_for_assign branch from 75bbdcc to cb47071 Jul 3, 2019

@cblecker

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

LGTM label has been added.

Git tree hash: f880db85dc4be061c84e6abef91052c64f288585

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, RainbowMango

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:

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

@k8s-ci-robot k8s-ci-robot merged commit ccfbc52 into kubernetes:master Jul 3, 2019

6 checks passed

cla/linuxfoundation RainbowMango authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped.
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-yamllint Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.