prow/plugins/blunderbuss: add /auto-cc command #10537

ibrasho · 2018-12-21T11:50:46Z

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

Resolve #9519

k8s-ci-robot · 2018-12-21T11:50:53Z

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 · 2018-12-21T12:15:52Z

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 · 2018-12-21T12:28:08Z

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 · 2018-12-21T12:31:24Z

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

stevekuznetsov

Cool idea! Some comments on the impl

prow/plugins/blunderbuss/blunderbuss.go

prow/plugins/blunderbuss/blunderbuss_test.go

stevekuznetsov · 2018-12-21T17:52:15Z

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 · 2018-12-21T17:52:56Z

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

prow/plugins/blunderbuss/blunderbuss.go

stevekuznetsov

Looking really nice! Small comments left.

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

prow/plugins/blunderbuss/blunderbuss_test.go

ibrasho · 2018-12-24T23:34:36Z

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 · 2018-12-24T23:35:14Z

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

idealhack · 2019-01-04T19:00:24Z

/ok-to-test

stevekuznetsov · 2019-01-04T21:11:06Z

@cjwagner @cblecker any thoughts?

cjwagner

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 · 2019-01-04T21:40:51Z

+1 for /auto-cc

ibrasho · 2019-01-04T22:20:45Z

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

ibrasho · 2019-01-04T22:21:11Z

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

cjwagner · 2019-01-04T22:34:38Z

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

prow/ANNOUNCEMENTS.md

stevekuznetsov · 2019-01-09T18:53:24Z

Needs a rebase, otherwise LGTM

idealhack · 2019-01-12T08:45:55Z

ping @ibrasho for rebasing

k8s-ci-robot · 2019-01-12T08:54:54Z

[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:

~~prow/OWNERS~~ [cjwagner]

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

ibrasho · 2019-01-12T09:23:13Z

Rebased.

idealhack · 2019-01-12T09:24:24Z

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 · 2019-01-12T16:00:17Z

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

Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

idealhack · 2019-01-12T16:21:31Z

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

/lgtm

k8s-ci-robot · 2019-01-12T16:21:33Z

LGTM label has been added.

Git tree hash: 44e287bac873059b5703a09a740bb2737a30c9b4

k8s-ci-robot added 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 Dec 21, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/prow Issues or PRs related to prow labels Dec 21, 2018

k8s-ci-robot requested review from cjwagner and stevekuznetsov December 21, 2018 11:51

stevekuznetsov reviewed Dec 21, 2018

View reviewed changes

ibrasho force-pushed the add-auto-assign-command branch 2 times, most recently from 2aa0e75 to 0827f59 Compare December 21, 2018 21:45

ibrasho commented Dec 21, 2018

View reviewed changes

prow/plugins/blunderbuss/blunderbuss.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Dec 24, 2018

View reviewed changes

ibrasho force-pushed the add-auto-assign-command branch from ba42de0 to 60188d5 Compare December 24, 2018 23:41

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 24, 2018

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 Jan 4, 2019

ibrasho force-pushed the add-auto-assign-command branch from 60188d5 to ecf5451 Compare January 4, 2019 19:07

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jan 4, 2019

ibrasho force-pushed the add-auto-assign-command branch from ecf5451 to 5ca930a Compare January 4, 2019 19:13

cjwagner approved these changes Jan 4, 2019

View reviewed changes

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

stevekuznetsov reviewed Jan 4, 2019

View reviewed changes

prow/ANNOUNCEMENTS.md Outdated Show resolved Hide resolved

ibrasho force-pushed the add-auto-assign-command branch 2 times, most recently from f07f9ec to 2f94e31 Compare January 4, 2019 23:34

ibrasho force-pushed the add-auto-assign-command branch from 2f94e31 to d6eab93 Compare January 12, 2019 08:54

ibrasho force-pushed the add-auto-assign-command branch from d6eab93 to 29bc97e Compare January 12, 2019 15:57

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

ibrasho force-pushed the add-auto-assign-command branch from 29bc97e to 665402d Compare January 12, 2019 15:59

ibrasho added 4 commits January 12, 2019 19:17

prow/plugins/blunderbuss: add /auto-cc command

c726550

Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

check for /auto-cc text and revert changes to handle

ac4aa0b

Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

prow/plugins/blunderbuss: refactor and add tests

ce029ae

Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

update ANNOUNCEMENTS.md

3f1628d

Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>

ibrasho force-pushed the add-auto-assign-command branch from 665402d to 3f1628d Compare January 12, 2019 16:17

k8s-ci-robot assigned idealhack Jan 12, 2019

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

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

ibrasho mentioned this pull request Jan 14, 2019

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

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prow/plugins/blunderbuss: add /auto-cc command #10537

prow/plugins/blunderbuss: add /auto-cc command #10537

ibrasho commented Dec 21, 2018 •

edited

Loading

k8s-ci-robot commented Dec 21, 2018

ibrasho commented Dec 21, 2018

ibrasho commented Dec 21, 2018

ibrasho commented Dec 21, 2018

stevekuznetsov left a comment

stevekuznetsov commented Dec 21, 2018

stevekuznetsov commented Dec 21, 2018

stevekuznetsov left a comment

ibrasho commented Dec 24, 2018

stevekuznetsov commented Dec 24, 2018

idealhack commented Jan 4, 2019

stevekuznetsov commented Jan 4, 2019

cjwagner left a comment

idealhack commented Jan 4, 2019

ibrasho commented Jan 4, 2019

ibrasho commented Jan 4, 2019

cjwagner commented Jan 4, 2019

stevekuznetsov commented Jan 9, 2019

idealhack commented Jan 12, 2019

k8s-ci-robot commented Jan 12, 2019

ibrasho commented Jan 12, 2019

idealhack commented Jan 12, 2019 •

edited

Loading

ibrasho commented Jan 12, 2019 •

edited

Loading

idealhack commented Jan 12, 2019

k8s-ci-robot commented Jan 12, 2019

prow/plugins/blunderbuss: add /auto-cc command #10537

prow/plugins/blunderbuss: add /auto-cc command #10537

Conversation

ibrasho commented Dec 21, 2018 • edited Loading

k8s-ci-robot commented Dec 21, 2018

ibrasho commented Dec 21, 2018

ibrasho commented Dec 21, 2018

ibrasho commented Dec 21, 2018

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov commented Dec 21, 2018

stevekuznetsov commented Dec 21, 2018

stevekuznetsov left a comment

Choose a reason for hiding this comment

ibrasho commented Dec 24, 2018

stevekuznetsov commented Dec 24, 2018

idealhack commented Jan 4, 2019

stevekuznetsov commented Jan 4, 2019

cjwagner left a comment

Choose a reason for hiding this comment

idealhack commented Jan 4, 2019

ibrasho commented Jan 4, 2019

ibrasho commented Jan 4, 2019

cjwagner commented Jan 4, 2019

stevekuznetsov commented Jan 9, 2019

idealhack commented Jan 12, 2019

k8s-ci-robot commented Jan 12, 2019

ibrasho commented Jan 12, 2019

idealhack commented Jan 12, 2019 • edited Loading

ibrasho commented Jan 12, 2019 • edited Loading

idealhack commented Jan 12, 2019

k8s-ci-robot commented Jan 12, 2019

ibrasho commented Dec 21, 2018 •

edited

Loading

idealhack commented Jan 12, 2019 •

edited

Loading

ibrasho commented Jan 12, 2019 •

edited

Loading