Page MenuHomePhabricator

Pass conduit credentials down to children workflow
ClosedPublic

Authored by vm on Aug 28 2014, 7:16 PM.

Details

Summary

Calling arc patch on a diff that's dependent on a different diff tries to patch the parent first.
To patch the parent a child workflow is created, and a conduit is passed down, but the credentials are not and it is not marked as authenticated.

Then when the child tries to get the commit message for the dependency, it checks isConduitAuthenticated() https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/ArcanistPatchWorkflow.php;2c3268f03ed70d3221eb1642bIc99ebb39b12902e$800 and on failure pops up an interactive editor for the commit message.

Instead we just pass down the credentials to the childred and mark them as authenticated, so this is not a problem.

Test Plan

With two diffs where DA2 depends on DA1, run arc patch --force --nobranch DA2 ... this no longer pops an interactive editor for the commit message for the dependency.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

vm updated this revision to Diff 24987.Aug 28 2014, 7:16 PM
vm retitled this revision from to Pass conduit credentials down to children workflow.
vm updated this object.
vm edited the test plan for this revision. (Show Details)
vm added reviewers: epriestley, chad.
hach-que added inline comments.
src/workflow/ArcanistPatchWorkflow.php
800–802 ↗(On Diff #24987)

Remove debugging code?

vm added inline comments.Sep 1 2014, 2:47 AM
src/workflow/ArcanistPatchWorkflow.php
800–802 ↗(On Diff #24987)

Oops. Thought I did.

vm updated this revision to Diff 25022.Sep 1 2014, 2:48 AM
vm edited edge metadata.

Remove debugging code

Ping, can we get this in soon? We'd like to support dependent diffs in our automated testing workflow (which relies on arc patch and hence needs diff).

chad resigned from this revision.Sep 5 2014, 12:46 AM
chad removed a reviewer: chad.
epriestley accepted this revision.Sep 8 2014, 2:42 PM
epriestley edited edge metadata.

Possibly conduitURI and forcedConduitVersion should be getting passed down too, although there is probably no effect in practice.

T2247 discusses a flavor of this case in more detail.

We also plan to simplify the login/credentials dance a bit (T5955).

Anyway, this seems fine for the moment.

This revision is now accepted and ready to land.Sep 8 2014, 2:42 PM
epriestley closed this revision.Sep 8 2014, 2:43 PM
epriestley updated this revision to Diff 25112.

Closed by commit rARC1b8ce98304f5 (authored by @vm, committed by @epriestley).