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/plugins/blunderbuss: add /auto-cc command #10537

Merged
merged 4 commits into from Jan 12, 2019

Conversation

Projects
None yet
5 participants
@ibrasho
Copy link
Member

ibrasho commented Dec 21, 2018

Adds a new /auto-cc command to blunderbuss to manually trigger assigning reviewers.

Resolve #9519

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 21, 2018

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

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Dec 21, 2018

This is my first functionality PR to test-infra, so I hope I didn't miss anything. 😁

I choose to refactor blunderbuss.handle instead of adding a new method. Not sure what's the preferred approach regarding this. (I couldn't determine a predominant style from the other PRs).

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Dec 21, 2018

I took the command name from @fejta's comment`, but I'm not sure it's the best option.

/(un)assign is used to add assignees while /(un)cc is used to manage reviewers.

Other options I thought about:

  • /auto-cc
  • /reassign-reviewers (or just /assign-reviewers)
@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Dec 21, 2018

Also, we alternatively could automatically trigger this for PRs older than X.

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

Cool idea! Some comments on the impl

Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss.go
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Dec 21, 2018

I choose to refactor blunderbuss.handle instead of adding a new method. Not sure what's the preferred approach regarding this. (I couldn't determine a predominant style from the other PRs).

As you're doing the same work for the two events -- creation and comment triggering -- I think your decision makes sense.

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Dec 21, 2018

Also, we alternatively could automatically trigger this for PRs older than X.

This is not an event-driven workflow and would be hard to implement as a plugin. If we need it, we can use the commenter in a periodic job to add the comments a la the stale issue closer

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch 2 times, most recently from 2aa0e75 to 0827f59 Dec 21, 2018

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

Looking really nice! Small comments left.

@cjwagner @cblecker did y'all want a look?

Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
Show resolved Hide resolved prow/plugins/blunderbuss/blunderbuss_test.go Outdated
@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Dec 24, 2018

One thing I've noticed is that t.Run(name, func(t *testing.T)) is not used in most plugins with table tests. Is there a reason for that?

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Dec 24, 2018

The concurrent test paradigm was recently added to testing library and those tests were written before -- no reason other than age :)

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from ba42de0 to 60188d5 Dec 24, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Dec 24, 2018

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Jan 4, 2019

/ok-to-test

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from 60188d5 to ecf5451 Jan 4, 2019

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from ecf5451 to 5ca930a Jan 4, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Jan 4, 2019

@cjwagner @cblecker any thoughts?

@cjwagner
Copy link
Member

cjwagner left a comment

Overall implementation looks good, but I don't like /auto-assign as the command, it is very misleading.

Blunderbuss does not assign users, rather it requests reviews from them. Today we have an /assign command for assigning users and a /cc command for requesting reviews from users. /auto-assign sounds like an automatic version of /assign, but this command really implements an automatic version of /cc so I think a name like /auto-cc would be more accurate and less misleading. If we were to implement an /auto-assign command I would expect it to assign a suggested OWNERS file approver.

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Jan 4, 2019

+1 for /auto-cc

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Jan 4, 2019

Personally, I'd prefer /auto-reviewers, but since we already use /cc to assign reviewers I've updated the command to /auto-cc.

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Jan 4, 2019

Is there any documentation that should be updated to reflect that we have a new command available for this?

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 4, 2019

Is there any documentation that should be updated to reflect that we have a new command available for this?

This will be used to generate plugin help information that is served from Prow's front end. https://github.com/kubernetes/test-infra/pull/10537/files#diff-ea943386c5188ebe891df8f3c91b56eeR68

You could also mention the new command in the "new features" section of ANNOUNCEMENTS.md: https://github.com/kubernetes/test-infra/blob/master/prow/ANNOUNCEMENTS.md#announcements

Show resolved Hide resolved prow/ANNOUNCEMENTS.md Outdated

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch 2 times, most recently from f07f9ec to 2f94e31 Jan 4, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Jan 9, 2019

Needs a rebase, otherwise LGTM

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Jan 12, 2019

ping @ibrasho for rebasing

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from 2f94e31 to d6eab93 Jan 12, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, ibrasho

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

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Jan 12, 2019

Rebased.

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Jan 12, 2019

Sorry but I forgot to mention that I don't think we need any mentions for auto-assign in the commits because I prefer a cleaner history, they are already here in the PR anyway, thoughts?

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from d6eab93 to 29bc97e Jan 12, 2019

@ibrasho ibrasho changed the title prow/plugins/blunderbuss: add /auto-assign command prow/plugins/blunderbuss: add /auto-cc command Jan 12, 2019

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from 29bc97e to 665402d Jan 12, 2019

@ibrasho

This comment has been minimized.

Copy link
Member Author

ibrasho commented Jan 12, 2019

I agree. I remove the extra commit and updated the previous ones.

ibrasho added some commits Dec 21, 2018

prow/plugins/blunderbuss: add /auto-cc command
Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>
check for /auto-cc text and revert changes to handle
Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>
prow/plugins/blunderbuss: refactor and add tests
Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>
update ANNOUNCEMENTS.md
Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

@ibrasho ibrasho force-pushed the ibrasho-forks:add-auto-assign-command branch from 665402d to 3f1628d Jan 12, 2019

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Jan 12, 2019

Thanks! Let's get it in before next rebasing notice. We can follow up if anything missed.

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 12, 2019

LGTM label has been added.

Git tree hash: 44e287bac873059b5703a09a740bb2737a30c9b4

@k8s-ci-robot k8s-ci-robot merged commit 743df73 into kubernetes:master Jan 12, 2019

12 checks passed

cla/linuxfoundation ibrasho authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details

@ibrasho ibrasho referenced this pull request Jan 14, 2019

Closed

REQUEST: New membership for @ibrasho #352

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment