1473127 - Add series landing support to the API

Status: RESOLVED FIXED

(Conduit :: Lando, enhancement)

Description

[Steven MacLeod [:smacleod]]

Lando API needs to support querying for and triggering landing on a revision stack.

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: stacks: add function to build a graph of a stack (Bug 1473127).

`build_stack_graph` takes a revision PHID and builds a graph
representing the entire stack it is part of. Any revision that is part
of the stack should result in an equal graph.

This function is expensive to call as it can make many requests to
Phabricator in a single call. In the future we might want to consider
caching things where possible.

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: revisions: refactor endpoint to prep for stacks (Bug 1473127).

This change refactors how the data for a revision is generated and
serialized to make it easier to implement stack support. Parts of
revisions that are needed for stacks have been moved to their own
functions.

This refactor also takes care of removing the use of
differential.querydiffs, transitioning completely to the commits
attachment of differential.diff.search. PhabricatorDouble now supports
this attachment and differential.querydiffs support has been removed to
discourage any future use.

Depends on D2087.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: stacks: add stacks endpoint (Bug 1473127).

This introduces the basic stack querying endpoint. There are three main
components at this time: The revisions that are part of the stack, the
edges between each revision forming the stack graph, and a list of
"landable paths".

A landable path is a series of revisions forming a line through the
stack DAG. Each of these landable paths meets a set of criteria required
for landing (such as not containing revisions with more than one open
parent).

Data on the status of each revision and specific repositoriy data for
each revision has been left out as it is not needed at this time.
Introducing more fields to the endpoint for this data will come in a
future change.

Depends on D2088.

Comment 4

[Phabricator Automation]

Comment on attachment 8991504 [details]
stacks: add function to build a graph of a stack (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

https://phabricator.services.mozilla.com/D2087

Comment 5

[Phabricator Automation]

Comment on attachment 8991505 [details]
revisions: refactor endpoint to prep for stacks (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

https://phabricator.services.mozilla.com/D2088

Comment 6

[Phabricator Automation]

Comment on attachment 8991506 [details]
stacks: add stacks endpoint (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

https://phabricator.services.mozilla.com/D2089

Comment 7

[Steven MacLeod [:smacleod]]

Attachment: stacks: add revision status (Bug 1473127).

Comment 8

[Steven MacLeod [:smacleod]]

Attachment: stacks: expose why revisions are blocked (Bug 1473127).

This change exposes blocker data through the stack API for revisions
which don't appear in a landable path. Each revision that is not
landable has a reason for being blocked, this is sent in the
"blocked_reason" field for each revision.

The landable sugraphs calculation has been updated to track this blocker
information and now returns it. An optional `other_checks` parameter has
been added to the function to support future non graph based reasons
for a revision being blocked. The argument should be an iterable of
callables which will be run against revisions to determine if they
should be blocked. This design was chosen to make testing both the
landable sugraph calculation and future blocker checks easier.

The downside of the current design means any blocker checks that depend
upon the structure of the graph must be part of the main algorithm. I
couldn't think of any other graph structure checks we'd ever want and
this helped simplify things considerably.

Depends on D3267.

Comment 9

[Steven MacLeod [:smacleod]]

Attachment: transplants: add dryrun endpoint (Bug 1473127).

This change provides a new transplant dryrun endpoint, laying out the
initial structure. Some features are missing, such as the proper
generation of a confirmation token and further blockers / warnings, but
enough is provided to allow UI to be developed. Future changes will
iterate on this endpoint and integrate it with the new Transplant
submission endpoint.

Since this new dryrun deals with many revisions at once, it aggregates
warning by type while still allowing details particular to a single
revision. The idea behind this being that a UI may show and allow
acknowledging the warnings of a single type all at once, rather than
individually for every revision with that warning. It is still possible
to drill down into specific details if needed.

Depends on D3268.

Comment 10

[Steven MacLeod [:smacleod]]

Attachment: db: migrate from Landing to Transplant (Bug 1473127).

This change migrates the codebase from the Landing model to a new
Transplant model which can support landings involving multiple
revisions. As part of the migration all previous Landings will be
represented with a new equivalent Transplant entry. We consciously throw
away Landing.active_diff_id as we no longer support landing "inactive"
diffs and the history isn't important.

The migration leaves the old Landing database table and data around as a
safety net. It can be cleaned up in a future change after this has been
successfully deployed.

All uses of Landing have been replaced with equivalent Transplant usage.
This will allow the current single revision Lando to operate while the
new series support is tested / developed. Eventually when series support
is ready to completely replace the current API legacy code may be
removed.

The most interesting difference between Landing and Transplant is the
replacement of `revision_id` and `diff_id` with `revision_to_diff_id`
and `revision_order`. `revision_to_diff_id` is a JSONB field, the
contents being a JSON object mapping revision ids to the diff id used
for that revision. This allows a single Transplant row to contain
indexable, queryable information regarding the set of revisions in a
series. Particularly it is possible to query for all Transplants that
contain any revision from a set (such as a stack). The `revision_order`
JSONB field is meant to support `revision_to_diff_id`, providing the
ordering for the revisions in the Transplant. This is needed since JSONB
objects do not maintain key ordering.

Depends on D3269.

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: transplants: add endpoint to query by stack (Bug 1473127).

GET /transplants will now allow querying for all transplants for an
entire stack, requiring a single revision ID from the stack be given.
This will allow UI to display any current or previous landings for a
stack.

Depends on D3270.

Comment 12

[:glob ✱]

Comment on attachment 8999779 [details]
stacks: add revision status (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 13

[:glob ✱]

Comment on attachment 8999780 [details]
stacks: expose why revisions are blocked (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 14

[:glob ✱]

Comment on attachment 8999783 [details]
db: migrate from Landing to Transplant (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 15

[:glob ✱]

Comment on attachment 8999781 [details]
transplants: add dryrun endpoint (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 16

[:glob ✱]

Comment on attachment 9000096 [details]
transplants: add endpoint to query by stack (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 17

[Steven MacLeod [:smacleod]]

Attachment: transplants: make warnings equivalent to landings' (Bug 1473127).

This change adds a few RevisionWarningChecks to make the transplants
warnings equivalent to those checked by landings.

Depends on D3368.

Comment 18

[Steven MacLeod [:smacleod]]

Attachment: stacks: make blockers equivalent to landings' (Bug 1473127).

This change adds a couple of new `other_checks` to the landable
subgraphs calculation calls to block revisions that would have been
blocked from landing. The blocker checks between transplanting a stack
and landing a revision should be mostly equivalent now.

Depends on D3898.

Comment 19

[Steven MacLeod [:smacleod]]

Attachment: transplants: add confirmation_token generation (Bug 1473127).

Depends on D3906.

Comment 20

[Steven MacLeod [:smacleod]]

Attachment: transplants: add POST endpoint to request transplant (Bug 1473127).

This change introduces the POST handler to /transplants, making it
possible to land a series of revisions. This is the final Lando API
change needed to support series landing.

Depends on D3909.

Comment 21

[:glob ✱]

Comment on attachment 9002802 [details]
transplants: make warnings equivalent to landings' (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 22

[:glob ✱]

Comment on attachment 9002819 [details]
stacks: make blockers equivalent to landings' (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 23

[:glob ✱]

Comment on attachment 9002847 [details]
transplants: add confirmation_token generation (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.

Comment 24

[:glob ✱]

Comment on attachment 9003297 [details]
transplants: add POST endpoint to request transplant (Bug 1473127).

Byron Jones ‹:glob› 🎈 has approved the revision.