Closed Bug 1539745 Opened 7 years ago Closed 7 years ago

HarborMaster build plan for code review bot

Categories

(Conduit :: Administration, task, P2)

Product:
Conduit
Component:
Administration
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bastien, Assigned: smacleod)

Details

(Keywords: conduit-triaged)

Since our instances of Phabricator now support non-blocking build plans, the code review bot can use HarborMaster build plans to signal its state to developpers.

I would need a Build plan on the production instance with the following attributes:

  • A single build step as a POST HTTP query with the following query path:
    ?build=${build.id}&diff=${buildable.diff}&target=${target.phid}&repository=${repository.phid}
  • The endpoint is not yet available (issue on our side), but you can use https://static-analysis.moz.tools/ for now
  • The HTTP build step must be using Wait for message as completion callback
  • An herald rule should trigger the build plan for all new non-secure Diffs including a change on dom/media. We'll remove the file path restriction when we are happy with initial test results.

Thanks !

Assignee: nobody → dkl
Status: NEW → ASSIGNED

I have created the necessary build plan and herald rules. Please check over them.

Build Plan:
https://phabricator.services.mozilla.com/harbormaster/plan/4/

Herald Rule:
https://phabricator.services.mozilla.com/H130

I will leave this bug open until it is verified that these are working as intended.

Flags: needinfo?(bastien)

I've disabled the herald rule to trigger the build plan for now, as we need to adjust some of the settings around draft promotion etc.

(In reply to Bastien Abadie [:bastien] from comment #0)

Since our instances of Phabricator now support non-blocking build plans, the code review bot can use HarborMaster build plans to signal its state to developpers.

I would need a Build plan on the production instance with the following attributes:

What name would you like for this build plan? This will be what is displayed to the user on the revision page.

Assignee: dkl → smacleod

Thanks for your reactivity guys, i'm not yet ready to deploy the webhook endpoint on our side (PR should be merged on monday/tuesday).

Regarding the build plan's name, i would suggest something simple like Code review or Mozilla code review. Sylvestre should decide on this though...

Flags: needinfo?(bastien) → needinfo?(sledru)

Ok so after talking with the upstream Phabricator people about how to properly do this, it was recommended that the bot should perform some more logic on its end to check whether it needs to run. It turns out to be more complicated that originally thought to only fire the build plan when a revision is not private on the upload of a new diff. The complication stems mostly from how we do security of revisions using a BMO daemon. All new revisions are private by default until BMO connects and opens them up. By then the diff is already attached.

So would it be possible that we just send you a notification on all new revisions and diffs, and then your bot code checks the revision to see if it is visible or not and if so then executes the tests? There is a Conduit API that can be used to query the revision for the needed details. Mostly though if the bot account being used has no special permissions, it is simple a test to see if the bot can see the revision or not.

Thoughts?

Flags: needinfo?(bastien)

Sure, it's not a problem. We already have an extensive integration with Conduit API, so it's just a matter of adding a new endpoint in our client and using it.

Should the bot query the endpoint a few times, to avoid getting in a race condition on the security level (if the bot calls the API before BMO daemon updates the revision) ? We will skip all security revisions for now.

Flags: needinfo?(bastien)

In term of naming, what about
Source code analysis or Automatic source code analysis
like the component in bugzilla?

Flags: needinfo?(sledru)

I looked a bit more into the Conduit API regarding secure revisions, here is what i can do:

  1. When the bot start, load all the projects with the secure-revision tag (on phabricator-dev, that would be PHID-PROJ-f2a3wl5wxtqdtfgdjqzk
  2. When the bot receive an HTTP notification, it loads the revision informations + linked projects through differential.revision.search.
  3. If a secure-revision project is in the list, it does not process the diff
  4. (optional) Retry X times after a period of Y seconds to check if the project list evolved.

Does that sound OK to you David ?

Flags: needinfo?(dkl)

(In reply to Bastien Abadie [:bastien] from comment #7)

I looked a bit more into the Conduit API regarding secure revisions, here is what i can do:

  1. When the bot start, load all the projects with the secure-revision tag (on phabricator-dev, that would be PHID-PROJ-f2a3wl5wxtqdtfgdjqzk
  2. When the bot receive an HTTP notification, it loads the revision informations + linked projects through differential.revision.search.
  3. If a secure-revision project is in the list, it does not process the diff
  4. (optional) Retry X times after a period of Y seconds to check if the project list evolved.

Does that sound OK to you David ?

Could work if we add the bot account to the privacy groups in BMO otherwise it will be able to query the API and it will just encounter an error exception that it cannot see the revision.

If we go with a bot with no permissions (preferable) then it would need to continue to query the revision for a max amount time to allow for BMO to do its job of updating the new revision. Problem is it could be a second or two or a few mins depending on server load.

Flags: needinfo?(dkl)
Keywords: conduit-triaged
Priority: -- → P2

(In reply to David Lawrence [:dkl] from comment #8)

(In reply to Bastien Abadie [:bastien] from comment #7)

I looked a bit more into the Conduit API regarding secure revisions, here is what i can do:

  1. When the bot start, load all the projects with the secure-revision tag (on phabricator-dev, that would be PHID-PROJ-f2a3wl5wxtqdtfgdjqzk
  2. When the bot receive an HTTP notification, it loads the revision informations + linked projects through differential.revision.search.
  3. If a secure-revision project is in the list, it does not process the diff
  4. (optional) Retry X times after a period of Y seconds to check if the project list evolved.

Does that sound OK to you David ?

Could work if we add the bot account to the privacy groups in BMO otherwise it will be able to query the API and it will just encounter an error exception that it cannot see the revision.

If we go with a bot with no permissions (preferable) then it would need to continue to query the revision for a max amount time to allow for BMO to do its job of updating the new revision. Problem is it could be a second or two or a few mins depending on server load.

The only problem here is that we are running on Heroku, which means we can retry only up to 30 seconds.
Unless we reply 200 to all requests and put them in a queue that we process in a background worker.

I guess we don't want to reply 200 unless we actually trigger a build though, right? Does what we reply to the Harbormaster request make a difference?

Another question. What would exactly happen in the Phabricator UI for secure revisions? Will it show a build starting and never finishing?

(In reply to Marco Castelluccio [:marco] from comment #9)

The only problem here is that we are running on Heroku, which means we can retry only up to 30 seconds.
Unless we reply 200 to all requests and put them in a queue that we process in a background worker.

(We will move out of Heroku at some point though, so this point is not so important)

I deployed my latest version of pulselistener on our testing environment, using the Code Review Bot account provided by David; and triggered some builds from phabricator-dev:

  • A secure revision does not trigger a codre review (after 4 retries) and gets a 403 response with Secure revision text message
  • A public revision triggers a code review and gets a 200 response with Task queued

I'll make more tests today, then i think we can merge this initial implementation on our side.

(In reply to Bastien Abadie [:bastien] from comment #11)

I deployed my latest version of pulselistener on our testing environment, using the Code Review Bot account provided by David; and triggered some builds from phabricator-dev:

  • A secure revision does not trigger a codre review (after 4 retries) and gets a 403 response with Secure revision text message
  • A public revision triggers a code review and gets a 200 response with Task queued

I'll make more tests today, then i think we can merge this initial implementation on our side.

So, looking at the secure revision, I can answer my question from comment 9.
The build is shown as started and failed, so the UX is not so great :(

Given this behavior would be the same if we replied 200 and then reported a failed state, maybe we should do this so that we can process requests in the background and avoid the 30s limit.

(In reply to Marco Castelluccio [:marco] from comment #12)

(In reply to Bastien Abadie [:bastien] from comment #11)

I deployed my latest version of pulselistener on our testing environment, using the Code Review Bot account provided by David; and triggered some builds from phabricator-dev:

  • A secure revision does not trigger a codre review (after 4 retries) and gets a 403 response with Secure revision text message
  • A public revision triggers a code review and gets a 200 response with Task queued

I'll make more tests today, then i think we can merge this initial implementation on our side.

So, looking at the secure revision, I can answer my question from comment 9.
The build is shown as started and failed, so the UX is not so great :(

Given this behavior would be the same if we replied 200 and then reported a failed state, maybe we should do this so that we can process requests in the background and avoid the 30s limit.

We can change the build step of the build plan to continue normally when a HTTP post has been sent. This will show as the build passed in the revision even if the code review bot didnt finish yet. Right now it set to Wait For Message and so you get the red build failure when the code review bot says it can't run because the revision is secure.

I can see if it is possible via the API to set a previous build run from passed to failed which maybe code-review-bot can set once it is has fully ran for a public revision.

Would this be better UX?

I don't think that would work with the current state of Phabricator.

When i tested updating a build status, i noticed that only the first final state (pass|fail) was displayed on the Diff page.
Any update after that is registered, and displayed in the build details, but not on the diff page. Developpers would not see the updates then...

I think the main UX problem is that we are forced to show a build failure on secure revisions, this could be confusing to developers. It would be better not to show the build at all on secure revisions, as we are not running the bot on those.

We just released the webhook in production, with updated DNS.

The build plan in production should have one build step, with the following configuration:

  • URI: https://eventlistener.moz.tools/codereview/new?diff=${buildable.diff}&repo=${repository.phid}&build=${build.id}&target=${target.phid}&revision=${buildable.revision}
  • Method POST
  • When complete : Wait For Message

The testing endpoint is used on phabricator-dev, on my build plan

Build plan has been updated with changed name and URL. Herald rule has also been modified to run for all changes but is still disabled for now.

Herald rule now enabled, bot should be receiving notifications.

This is working now.

Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.