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

Fix project plugin: only process comment if matches regex #12125

Merged

Conversation

@taragu
Copy link
Member

taragu commented Apr 8, 2019

@taragu

This comment has been minimized.

Copy link
Member Author

taragu commented Apr 8, 2019

/cc @cjwagner

@k8s-ci-robot k8s-ci-robot requested a review from cjwagner Apr 8, 2019

@taragu taragu force-pushed the taragu:fix-project-plugin-endless-comments branch from 9335c0c to 2329ac4 Apr 9, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS labels Apr 9, 2019

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

@taragu Can we also add a specific test case of a comment that doesn't match /project at all? Otherwise LGTM.

@taragu taragu force-pushed the taragu:fix-project-plugin-endless-comments branch from 2329ac4 to 07accca Apr 9, 2019

@taragu

This comment has been minimized.

Copy link
Member Author

taragu commented Apr 9, 2019

@cblecker sure! Just added the test case

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

/lgtm
/approve
/hold

Will leave for @stevekuznetsov or @cjwagner to review as well.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 9, 2019

LGTM label has been added.

Git tree hash: f8f7a21357d799a20a8a5ae3c0cdf9fe19f96ccc

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Apr 9, 2019

We also want to explicitly ignore comments from the robot itself, right?

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

@stevekuznetsov It looks like only sigmention and trigger do that kind of validation, where as the rest of the plugins will act on any comment the bot makes. Is there any use case for the bot reacting to itself? Perhaps that validation would better live upstream of the plugins themselves.

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

@taragu Yeah, let's go ahead and add one more safeguard against the bot reacting to itself. We're still not sure if this should be an across the board thing, but we want to tread a little lighter.

botName, err := gc.BotName()
if err != nil {
return err
}
if e.User.Login == botName {
return nil
}

@taragu taragu force-pushed the taragu:fix-project-plugin-endless-comments branch from 07accca to 390dbc3 Apr 10, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 10, 2019

@cblecker
Copy link
Member

cblecker left a comment

One last nit: Can these three different short circuits be code commented?

Show resolved Hide resolved prow/plugins/project/project.go
Show resolved Hide resolved prow/plugins/project/project.go
Show resolved Hide resolved prow/plugins/project/project.go

@taragu taragu force-pushed the taragu:fix-project-plugin-endless-comments branch from 390dbc3 to fc2a1f0 Apr 10, 2019

@taragu

This comment has been minimized.

Copy link
Member Author

taragu commented Apr 10, 2019

@cblecker sounds good. Just make the update

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 10, 2019

/lgtm
/approve

/assign @stevekuznetsov
for approval

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 10, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 10, 2019

LGTM label has been added.

Git tree hash: 8dc43212bea6d58429022036930a2d5dbe7fb605

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

@taragu could you add unit tests to TestProjectCommand for:

  • no action on events that are not new comments
  • no action on bot comments
  • no action on non-matching comments

@taragu taragu force-pushed the taragu:fix-project-plugin-endless-comments branch from fc2a1f0 to b9698a0 Apr 12, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed lgtm size/M labels Apr 12, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Apr 15, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Apr 15, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 15, 2019

LGTM label has been added.

Git tree hash: 9947312826d3770258c2a6be27a4c8c755d98412

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, stevekuznetsov, taragu

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 c700e78 into kubernetes:master Apr 15, 2019

14 checks passed

cla/linuxfoundation taragu 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-file-perms Job succeeded.
Details
pull-test-infra-verify-github-spelling Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-labels Skipped.
pull-test-infra-verify-tslint Skipped.
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.