search

securesauce / precaution-beta

Precaution provides a simple, automated code review for GitHub projects by running code linters with a security focus on pull requests.
Other
1 stars 0 forks source link

Remove spurious handling of CheckSuiteEvent's requested action #199

Closed MVrachev closed 5 years ago

MVrachev commented 5 years ago

We don't need to respond to CheckSuiteEvent when the action is requested. We are already listening to the events:

Our event handling covers the following scenarios:

Fixes: #169

Signed-off-by: Martin Vrachev mvrachev@vmware.com

MVrachev commented 5 years ago

I made a detailed analysis of the different occasions what events we receive:

I will be calling the forked repo with F and original repo with O.


Pull request from repo to itself

  1. check_suite.requested
  2. pull_request.opened
  3. check_run with in-progress status is created
  4. List pull request files
  5. Download all of the files
  6. Send results

Pull request from F repo to the O repo with Precaution installed on both F and original O

  1. check_suite.requested
  2. pull_request.opened
  3. check_run with in-progress status is created
  4. List pull request files
  5. Download all of the files
  6. Send results

Pull request from F repo to O repo with Precaution is installed only on the F repo

Here is the bug from issue #169

When you initially create a pull request no events are happening. But when you update the pull request with a commit here are the events:

  1. check_suite.requested
  2. check_run with in-progress status is created
  3. List pull request files which ERRORS
  4. Send ERROR results

It’s curious that there is no pull_request.opened event here.

When you click “Re-run-all-checks”/“Re-run” button on the UI at already opened pull request

This is a pr from repo to itself

  1. check_suite.rerequested
  2. check_run with in-progress status is created
  3. List pull request files
  4. Download all of the files
  5. Send results
ericwb commented 5 years ago

Please update the PR and commit title. "Fix issue 169" is not descriptive enough and is best only included at the end of the commit message.

MVrachev commented 5 years ago

Update commit/PR message

What do you mean to update the pr/commit message? I think I explained the problem a lot in the message itself.

joshuagl commented 5 years ago

Accidentally closed, apologies.

joshuagl commented 5 years ago

Ideally a commit message shouldn't require a reader to refer to an external link, the "Fixes: #foo" reference should allow the reader to find more context if required but shouldn't be mandatory reading.

Can we update the commit message here to be more self-contained? Perhaps something like:

Remove spurious handling of CheckSuiteEvent's requested action

We don't need to respond to CheckSuiteEvent when the action is requested. We are already listening to the events pull_request events and check_suite.rerequested event which are enough for us to do our work. Our event handling covers the following scenarios:

  • pull request from forked repo to the original repo
  • pull request from repo to itself
  • when clicked re-run-all-checks/re-run buttons on the UI on already opened pull request

Fixes: #169

joshuagl commented 5 years ago

You have the "Fixes: #169" tag twice. Best to include it only once, at the bottom, before the signed-off-by.

Once that's fixed, LGTM.