Call conduit over HTTP #60
Conversation
ascandella
force-pushed
the
conduit-http
branch
2 times, most recently
from
6dc74f5
to
82b481c
Compare
|
cc @cburroughs I think the only thing that's left to satisfy the needs of the "generic conduit" task is to omit the checking for |
|
I'll have to re-integrate on my local stack, but broadly I think this looks good and in the right direction. |
|
(Oh and I like how the credentials changes side-step the shared config problem.) |
|
@jjx can you help review? It's kind of a monster so if you don't have time I can self review, but would like to get some sort of sanity check here. |
This removes much of the reliance on `arc` being installed. Also refactor apply patch into a task -- I realize this should be a separate commit, but all of this stuff is intertwined and I wanted to break it out to make this refactor easier.
| .stdout(logger.getStream()) | ||
| .cmds(Arrays.asList("git", "submodule", "update", "--init", "--recursive")) | ||
| .join(); | ||
| DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); |
There was a problem hiding this comment.
Just initialize this inside the try since it's not used anywhere else.
| public HttpUriRequest createRequest(String action, Map<String, String> data) throws UnsupportedEncodingException, ConduitAPIException { | ||
| HttpPost post; | ||
| try { | ||
| post = new HttpPost( |
There was a problem hiding this comment.
Can we do new URL(String.format("%s/api/%s", baseUrl, action)) instead?
There was a problem hiding this comment.
I had that originally, but when testing manually I was running into issues with double-slashes, e.g. "http://phabricator.testing//api/" when the user inputted a trailing slash. Rather than doing manual string munging, I thought I'd just use URL, but I agree that this is awkward. Is there a better way?
There was a problem hiding this comment.
I see.. I guess this is a mistake proof.
ascandella
force-pushed
the
conduit-http
branch
from
66b4ff5
to
fd71a4f
Compare
| e.getMessage(), | ||
| query.toString(2))); | ||
| } | ||
| try { | ||
| return response.getJSONObject(diffID); | ||
| } catch (JSONException e) { | ||
| throw new ArcanistUsageException( | ||
| throw new ConduitAPIException( | ||
| String.format("Unable to find '%s' key in response: (%s) %s", |
| .cmds(Arrays.asList(gitPath, "submodule", "update", "--init", "--recursive")) | ||
| .join(); | ||
|
|
||
| List<String> params = new ArrayList<String>(Arrays.asList("--nobranch", "--diff", diffID)); |
There was a problem hiding this comment.
arcPatchParams maybe to be more specific?
|
@jjx I think I addressed all your feedback; ready for another review |
|
🍏 |
This removes much of the reliance on
arcbeing installed.Also refactor apply patch into a task -- I realize this should be a separate
commit, but all of this stuff is intertwined and I wanted to break it out to
make this refactor easier.
Supersedes #55