1699137 - Add an API to upload custom warnings

Status: RESOLVED FIXED

(Conduit :: Lando, enhancement, P3)

Description

[Marco Castelluccio [:marco]]

The reviewbot posts lint issues and build errors to Phabricator via harbormaster (see https://github.com/mozilla/code-review/blob/94e69637eb78fb7d0baa422775cfb29d2048cfa0/bot/code_review_bot/report/phabricator.py#L87 and https://github.com/mozilla/libmozdata/blob/2adac31e500a38669344e946ac8009b99738ca0c/libmozdata/phabricator.py#L479).

Some of them, e.g. clang-format or clang-tidy, can safely be ignored by developers as we know there are pre-existing issues in the codebase and their tasks are allowed to fail on mozilla-central.

Some other, e.g. flake8, can't be ignored by developers. If they land a patch with flake8 errors, the flake8 mozilla-central task will fail and the patch will be backed-out (or the developer will have to land a bustage fix).

Currently, Lando doesn't use the issues published by the reviewbot in any way. It'be nice if, in case of issues from unignorable linters, Lando prevented the landing of the stack or, at least, showed a warning that the developer must aknowledge before landing.

The only caveat is that there are (few) cases where the developer will try to land the patch before the reviewbot is finished analyzing, and so Lando is not always able to know if there are issues linked to the patch. I think this is acceptable though, as it should be relatively rare.

Comment 1

[Andrew Halberstadt [:ahal]]

+1

In the case of unfinished tasks, I think that is something the submitter should also have to acknowledge. Maybe they could be batched together:

[ ] Reviewbot has detected failures or there are unfinished tasks

I think failures shouldn't block landing outright as the revision may be based on top of pre-existing issues, or there may be infrastructure problems outside of version control.

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)

I think failures shouldn't block landing outright as the revision may be based on top of pre-existing issues, or there may be infrastructure problems outside of version control.

We have a way to detect issues that would cause backouts vs those that wouldn't (we report them with different severity levels on Phabricator), but yeah I agree it's preferable not to block landing altogether.

Comment 3

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)

[ ] Reviewbot has detected failures or there are unfinished tasks

Bonus points if this message also includes a link to the push :)

Comment 4

[Zeid [:zeid]]

Attachment: GitHub Pull Request

Three endpoints have been created to support this functionality. Here's one possible implementation on the client-side:

• create a new custom warning when a build/task is kicked off
• save the ID of the warning above, so it can be deleted later, or query all custom warnings in a particular group to fetch their IDs, then delete them (there should really only be one in this initial case.)
• if there are additional warnings based on the results, then delete all warnings and create a new one with a useful message
• if there are no more warnings, then delete all warnings and take no further action.

Notes:

• this PR is a work in progress, but the schema of the endpoints should not change much
• see this test for an example POST request
• there will be a form of authentication required before this is deployed to production

Comment 5

[Andrew Halberstadt [:ahal]]

Nice work Zeid! It would be great if the warnings could also be markdown formatted (if they aren't already) so we could link to the try push containing failures / unfinished tasks. Though this is a separate and lower priority request.

Comment 6

[Zeid [:zeid]]

With the current implementation, only links to Phabricator revisions and bugs will automatically linkify, since this is the current setup on the frontend. In the future, we can certainly allow for more customization. If you want, please file a a bug with an example warning message with links. Adding markdown support is probably much more complex than just allowing for specific links to be included in a warning (from a Lando UI perspective).

Comment 7

[Andrew Halberstadt [:ahal]]

Can this be closed now?

Comment 8

[Zeid [:zeid]]

yes