1487843 - Figure out a way to upload coverage data and report linting/static analysis results for Phabricator revisions

Status: RESOLVED FIXED

(Conduit :: General, enhancement, P2)

Description

[Marco Castelluccio [:marco]]

In the current development workflow, we don't have coverage data for each review request, as it would be pretty expensive to run try builds for all of them.

We think coverage data can be interesting for reviewers (and release managers) though, so we want to find a way to effectively show this data in Phabricator.

We are planning to update coverage data for:
1) landed revisions, for which we always have a coverage build (even if in aggregated form, but we can map back the coverage);
2) revisions for which a developer/reviewer requests a coverage try build;
3) (maybe) specific revisions that adhere to specific rules (e.g. patch in a specific component, or patch bigger than N lines, etc.).

Phabricator offers the harbormaster.sendmessage API to upload coverage data for a build target.

One way to get a build target is through harbormaster.queryautotargets, which, for an autoplan, either retrieves or creates an auto build target. For the time being, we could rely on the arcanist.unit autoplan, which is defined by default. In the future, it might be nice to have a separate autoplan specifically for code coverage (and maybe one specific for linting, for our static analysis bot results).

The other option is to define a build plan that submits a HTTP request to a backend of ours (similar to the Bugzilla one), which stores the build target ID. Then we can use this build target ID for the harbormaster.sendmessage API call.

I prefer the option with the autotarget, as it removes the need for yet another backend, making the overall architecture leaner, and it reduces the dependencies between the projects. I'm open to discussion though.

More details about how to upload coverage for a revision at https://secure.phabricator.com/T9529.

Comment 2

[Marco Castelluccio [:marco]]

For static analysis/linting maybe we will keep posting review comments instead of uploading lint issues as these can't be dropped like normal review comments, but we will use the build target anyway to let the developer know that the analysis started/is in progress/finished.

Comment 3

[Marco Castelluccio [:marco]]

Could you help with this? We have an interim solution (piggybacking on arcanist.unit target), but I can see it's not as clean as it could be. I suppose creating a new build autotarget requires changes on your side (similarly to what you've done for Bugzilla).

Comment 4

[Mark Côté [:mcote]]

Heya really sorry about getting back to you on this.  We're taking a look at the best solution for you this week.

Comment 5

[Steven MacLeod [:smacleod]]

Update: I'm waiting on feedback from Phacility on some of the options we have here (https://admin.phacility.com/PHI896). When that comes in we should be able to move forward.

Comment 6

[:glob ✱]

from phacility:

> I expect to have a "this build does not block promotion from the 'draft' state"
> option in Harbormaster "soon™". I think this isn't clearly captured by any
> single task right now, but I expect it to be a part of the next Harbormaster
> iteration in T13088.  Until then, I think (1) is probably the cleanest route
> forward -- then just remove the early success state once the bot is more stable
> and the "don't block promotion" checkbox exists.

where (1) is:

> Create an empty build plan, and a herald rule that runs it on every diff.
> Would immediately be in the success state.
> Gives the bot something to use for harbormaster.sendmessage.

Comment 7

[Steven MacLeod [:smacleod]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #6)
> where (1) is:
> 
> > Create an empty build plan, and a herald rule that runs it on every diff.
> > Would immediately be in the success state.
> > Gives the bot something to use for harbormaster.sendmessage.

Just to clarify, this will *not* be an "autoplan", but just a normal build plan we're triggering. Autoplan's require code extensions and Phacility recommended the empty standard build plan rather than an autoplan.

:marco, I'll need to know what you'll want to display for the coverage data build plan on the revision page (the name this build plan will have to indicate what it is).

:janx, I'll need the same thing from you, except for the static analysis plan ^.

Comment 8

[Jan Keromnes [:janx]]

Thanks for the update!

> :janx, I'll need the same thing from you, except for the static analysis plan ^.

We want to show developers what status the code analysis is in.

I guess we can call the status item "Code analysis" (or "Reviewbot code analysis" if we feel the former is too ambiguous).

If we can indicate status messages, I'd like to show:
- requested
- in progress
- found {N} defects
- failed

(We can possibly show more granular status, like the various steps analysis goes through, but "in progress" will probably be enough for now.)

Comment 9

[Marco Castelluccio [:marco]]

I was talking with smacleod via email, but let's summarize the current situation here since he's currently on PTO:

- We can't use it for static analysis, as the Phabricator UI would show the build directly in the finished state, and what we wanted is a way to show progress. We need to wait on an upcoming Phabricator feature that allows specifying build plans which can fail without blocking revisions from moving out of draft;

- We can, in theory, use it for coverage uploading (even though showing the build directly in the finished state is not great in terms of UX). In order to do that, we need to use the sendmessage API (as glob said in comment 6). The problem is that this API requires a build target PHID as input, but there seems no way to retrieve a build target PHID from a revision (https://discourse.phabricator-community.org/t/target-phid-to-report-build-status-back-to-harbormaster/1783/5).

Using an autoplan would solve both problems (the queryautotargets API can be used to retrieve a build target PHID on which sendmessage can be used), but Phacility didn't recomment it (maybe because autoplans are extensions of the source code).

Comment 10

[Marco Castelluccio [:marco]]

Hey Steven, any news? Did you decide between the three solutions you were thinking of? a) Phabricator extension that provides a new API endpoint to find a build target PHID given a diff PHID; b) trigger our bot using the built-in method, sending an HTTP request to us with the build target PHID; c) publish the mapping to an external system we can query.

Comment 11

[Steven MacLeod [:smacleod]]

Sorry for the delay, was off sick.

(In reply to Marco Castelluccio [:marco] from comment #10)
> Hey Steven, any news? Did you decide between the three solutions you were
> thinking of?
I'm waiting on one more response from Phacility to see if we can either get
the build `target.phid` added to `harbormaster.build.search` or more
information about using our own autoplan.

If we just want to get things going as quickly as possible I think it makes
sense to use the arcanist autoplan and stick the coverage information there
for now. Everything around the actual uploading of coverage data should
remain the same, you just might need to change the code that grabs the target
phid later.

As for the longer term solution, I did a bit more digging:

> a) Phabricator extension that provides a new API endpoint to
> find a build target PHID given a diff PHID;
It shouldn't be too too difficult to write this, maintenance is a little less
clear though. There seems to be a decent amount of harbormaster development
going on lately, but that also tells me they'll be more likely to add the
target phid to `harbormaster.build.search`. Assuming they won't upstream
what we need, I'd be willing to move forward here.

> b) trigger our bot using the
> built-in method, sending an HTTP request to us with the build target PHID;
This would be my preferred way forward if we can't get the target phid
exposed upstream. It *would* need to wait on the upstream work around builds
not blocking draft promotion, but that's in progress. I think this solution
makes sense as it allows for things like manually retriggering failed runs
in the Phabricator UI, or even manually triggering on revisions where we don't
automatically run.

Even if we don't go with this for coverage, I think it makes a lot of sense
for static analysis (again, waiting on draft promotion block setting).

(Leaving needinfo open until I've updated with Phacility response.)

Comment 12

[Steven MacLeod [:smacleod]]

(In reply to Steven MacLeod [:smacleod] from comment #11)
> > a) Phabricator extension that provides a new API endpoint to
> > find a build target PHID given a diff PHID;
> It shouldn't be too too difficult to write this, maintenance is a little less
> clear though. There seems to be a decent amount of harbormaster development
> going on lately, but that also tells me they'll be more likely to add the
> target phid to `harbormaster.build.search`. Assuming they won't upstream
> what we need, I'd be willing to move forward here.

Phacility got back to me and is going to implement what we need to query for target phids. First piece was https://secure.phabricator.com/D19818 and the rest will be coming in the future.

Comment 13

[Steven MacLeod [:smacleod]]

The upstream changes for not blocking draft promotion have landed, so I think our best bet is to go with the standard build plans making web requests. Marking this as FIXED since the investigation work of our options and the upstream work to enable them is done. I'll be meeting with :marco soon and we can file any followup work, such as getting build plans created, then.