From 0a342c7b7ab054614063d1522cecfff372c4f08f Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 9 Aug 2015 15:51:19 -0700 Subject: [PATCH 1/8] Call conduit over HTTP 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. --- phabricator-plugin.iml | 1 + pom.xml | 8 + .../phabricator/PhabricatorBuildWrapper.java | 144 +++++++++--------- .../phabricator/PhabricatorNotifier.java | 46 ++++-- .../phabricator/PhabricatorPlugin.java | 2 - .../phabricator/conduit/ConduitAPIClient.java | 96 ++++++++++++ .../conduit/ConduitAPIException.java | 35 +++++ .../phabricator/conduit/Differential.java | 3 +- .../conduit/DifferentialClient.java | 43 ++---- .../phabricator/tasks/ApplyPatchTask.java | 135 ++++++++++++++++ .../phabricator/tasks/PostCommentTask.java | 8 +- .../conduit/ConduitAPIClientTest.java | 92 +++++++++++ .../conduit/DifferentialClientTest.java | 21 ++- .../phabricator/tasks/ApplyPatchTaskTest.java | 60 ++++++++ .../tasks/PostCommentTaskTest.java | 4 +- .../jenkins/phabricator/utils/TestUtils.java | 28 +++- .../conduit/fetchDiffWithResponseArray.json | 3 + 17 files changed, 589 insertions(+), 140 deletions(-) create mode 100644 src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java create mode 100644 src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIException.java create mode 100644 src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java create mode 100644 src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java create mode 100644 src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java create mode 100644 src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json diff --git a/phabricator-plugin.iml b/phabricator-plugin.iml index cc0f613e..373c7a10 100644 --- a/phabricator-plugin.iml +++ b/phabricator-plugin.iml @@ -130,6 +130,7 @@ + diff --git a/pom.xml b/pom.xml index 8034cd76..2c4fc594 100644 --- a/pom.xml +++ b/pom.xml @@ -90,6 +90,14 @@ test 4.12 + + + org.apache.httpcomponents + httpclient + 4.3 + tests + test + diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index f51ea652..8091db70 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -20,11 +20,13 @@ package com.uber.jenkins.phabricator; -import com.uber.jenkins.phabricator.conduit.ArcanistClient; -import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; +import com.uber.jenkins.phabricator.conduit.ConduitAPIClient; +import com.uber.jenkins.phabricator.conduit.ConduitAPIException; import com.uber.jenkins.phabricator.conduit.Differential; import com.uber.jenkins.phabricator.conduit.DifferentialClient; import com.uber.jenkins.phabricator.credentials.ConduitCredentials; +import com.uber.jenkins.phabricator.tasks.ApplyPatchTask; +import com.uber.jenkins.phabricator.tasks.Task; import com.uber.jenkins.phabricator.utils.CommonUtils; import com.uber.jenkins.phabricator.utils.Logger; import hudson.EnvVars; @@ -36,9 +38,14 @@ import org.kohsuke.stapler.DataBoundConstructor; import java.io.IOException; -import java.util.*; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; public class PhabricatorBuildWrapper extends BuildWrapper { + private static final String CONDUIT_TAG = "conduit"; + private static final String DEFAULT_GIT_PATH = "git"; + private final boolean createCommit; private final boolean applyToMaster; private final boolean uberDotArcanist; @@ -52,6 +59,7 @@ public PhabricatorBuildWrapper(boolean createCommit, boolean applyToMaster, bool this.showBuildStartedMessage = showBuildStartedMessage; } + /** {@inheritDoc} */ @Override public Environment setUp(AbstractBuild build, Launcher launcher, @@ -62,93 +70,69 @@ public Environment setUp(AbstractBuild build, return this.ignoreBuild(logger, "No environment variables found?!"); } - final String arcPath = this.getArcPath(); - final Map envAdditions = new HashMap(); - envAdditions.put(PhabricatorPlugin.ARCANIST_PATH, arcPath); - - final String conduitToken = this.getConduitToken(build.getParent(), logger); String diffID = environment.get(PhabricatorPlugin.DIFFERENTIAL_ID_FIELD); if (CommonUtils.isBlank(diffID)) { this.addShortText(build); this.ignoreBuild(logger, "No differential ID found."); - } else { - LauncherFactory starter = new LauncherFactory(launcher, environment, listener.getLogger(), build.getWorkspace()); - - if (uberDotArcanist) { - int npmCode = starter.launch() - .cmds(Arrays.asList("npm", "install", "uber-dot-arcanist")) - .stdout(logger.getStream()) - .join(); - - if (npmCode != 0) { - logger.warn("uber-dot-arcanist", "Got non-zero exit code installing uber-dot-arcanist from npm: " + npmCode); - } - } - - DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); - Differential diff; - try { - diff = new Differential(diffClient.fetchDiff()); - diff.decorate(build, this.getPhabricatorURL(build.getParent())); - - logger.info("arcanist", "Applying patch for differential"); - - // Post a silent notification if option is enabled - if (showBuildStartedMessage) { - diffClient.postComment(diff.getRevisionID(false), diff.getBuildStartedMessage(environment)); - } - } catch (ArcanistUsageException e) { - logger.warn("arcanist", "Unable to apply patch"); - logger.warn("arcanist", e.getMessage()); - return null; - } + return new Environment(){}; + } - String baseCommit = "origin/master"; - if (!applyToMaster) { - baseCommit = diff.getBaseCommit(); - } + LauncherFactory starter = new LauncherFactory(launcher, environment, listener.getLogger(), build.getWorkspace()); - int resetCode = starter.launch() - .cmds(Arrays.asList("git", "reset", "--hard", baseCommit)) + if (uberDotArcanist) { + int npmCode = starter.launch() + .cmds(Arrays.asList("npm", "install", "uber-dot-arcanist")) .stdout(logger.getStream()) .join(); - if (resetCode != 0) { - logger.warn("arcanist", "Got non-zero exit code resetting to base commit " + baseCommit + ": " + resetCode); + if (npmCode != 0) { + logger.warn("uber-dot-arcanist", "Got non-zero exit code installing uber-dot-arcanist from npm: " + npmCode); } + } - // Clean workspace, otherwise `arc patch` may fail - starter.launch() - .stdout(logger.getStream()) - .cmds(Arrays.asList("git", "clean", "-fd", "-f")) - .join(); + ConduitAPIClient conduitClient; + try { + conduitClient = getConduitClient(build.getParent(), logger); + } catch (ConduitAPIException e) { + e.printStackTrace(logger.getStream()); + logger.warn(CONDUIT_TAG, e.getMessage()); + return null; + } - // Update submodules recursively. - starter.launch() - .stdout(logger.getStream()) - .cmds(Arrays.asList("git", "submodule", "update", "--init", "--recursive")) - .join(); + DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); + Differential diff; + try { + diff = new Differential(diffClient.fetchDiff()); + diff.decorate(build, this.getPhabricatorURL(build.getParent())); - List params = new ArrayList(Arrays.asList("--nobranch", "--diff", diffID)); - if (!createCommit) { - params.add("--nocommit"); - } + logger.info(CONDUIT_TAG, "Fetching differential from Conduit API"); - ArcanistClient arc = new ArcanistClient( - arcPath, - "patch", - null, - conduitToken, - params.toArray(new String[params.size()])); + // Post a silent notification if option is enabled + if (showBuildStartedMessage) { + diffClient.postComment(diff.getRevisionID(false), diff.getBuildStartedMessage(environment)); + } + } catch (ConduitAPIException e) { + logger.warn(CONDUIT_TAG, "Unable to apply patch"); + logger.warn(CONDUIT_TAG, e.getMessage()); + return null; + } - int result = arc.callConduit(starter.launch(), logger.getStream()); + String baseCommit = "origin/master"; + if (!applyToMaster) { + baseCommit = diff.getBaseCommit(); + } - if (result != 0) { - logger.warn("arcanist", "Error applying arc patch; got non-zero exit code " + result); - return null; - } + final String conduitToken = this.getConduitToken(build.getParent(), logger); + Task.Result result = new ApplyPatchTask( + logger, starter, baseCommit, diffID, conduitToken, getArcPath(), + DEFAULT_GIT_PATH, createCommit + ).run(); + + if (result != Task.Result.SUCCESS) { + logger.warn("arcanist", "Error applying arc patch; got non-zero exit code " + result); + return null; } return new Environment() { @@ -172,6 +156,18 @@ private Environment ignoreBuild(Logger logger, String message) { return new Environment(){}; } + private ConduitAPIClient getConduitClient(Job owner, Logger logger) throws ConduitAPIException { + ConduitCredentials credentials = getConduitCredentials(owner); + if (credentials == null) { + throw new ConduitAPIException("No credentials configured for conduit"); + } + return new ConduitAPIClient(credentials.getUrl(), getConduitToken(owner, logger)); + } + + private ConduitCredentials getConduitCredentials(Job owner) { + return getDescriptor().getCredentials(owner); + } + /** * This is used in config.jelly to populate the state of the checkbox */ @@ -196,7 +192,7 @@ public boolean isShowBuildStartedMessage() { } public String getPhabricatorURL(Job owner) { - ConduitCredentials credentials = this.getDescriptor().getCredentials(owner); + ConduitCredentials credentials = getConduitCredentials(owner); if (credentials != null) { return credentials.getUrl(); } @@ -204,7 +200,7 @@ public String getPhabricatorURL(Job owner) { } public String getConduitToken(Job owner, Logger logger) { - ConduitCredentials credentials = this.getDescriptor().getCredentials(owner); + ConduitCredentials credentials = getConduitCredentials(owner); if (credentials != null) { return credentials.getToken().getPlainText(); } diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java index 6550bc87..96c7a5c8 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java @@ -20,9 +20,7 @@ package com.uber.jenkins.phabricator; -import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; -import com.uber.jenkins.phabricator.conduit.Differential; -import com.uber.jenkins.phabricator.conduit.DifferentialClient; +import com.uber.jenkins.phabricator.conduit.*; import com.uber.jenkins.phabricator.credentials.ConduitCredentials; import com.uber.jenkins.phabricator.tasks.NonDifferentialBuildTask; import com.uber.jenkins.phabricator.tasks.PostCommentTask; @@ -48,6 +46,8 @@ import java.io.PrintStream; public class PhabricatorNotifier extends Notifier { + private static final String UBERALLS_TAG = "uberalls"; + private static final String CONDUIT_TAG = "conduit"; // Post a comment on success. Useful for lengthy builds. private final boolean commentOnSuccess; private final boolean uberallsEnabled; @@ -86,9 +86,6 @@ public final boolean perform(final AbstractBuild build, final Launcher lau environment.get("GIT_URL"), branch); final boolean needsDecoration = build.getActions(PhabricatorPostbuildAction.class).size() == 0; - final String conduitToken = getConduitToken(build.getParent(), logger); - - final String arcPath = environment.get(PhabricatorPlugin.ARCANIST_PATH, "arc"); final boolean uberallsConfigured = !CommonUtils.isBlank(uberalls.getBaseURL()); final String diffID = environment.get(PhabricatorPlugin.DIFFERENTIAL_ID_FIELD); @@ -107,14 +104,21 @@ public final boolean perform(final AbstractBuild build, final Launcher lau return true; } - LauncherFactory starter = new LauncherFactory(launcher, environment, listener.getLogger(), build.getWorkspace()); + ConduitAPIClient conduitClient; + try { + conduitClient = getConduitClient(build.getParent(), logger); + } catch (ConduitAPIException e) { + e.printStackTrace(logger.getStream()); + logger.warn(CONDUIT_TAG, e.getMessage()); + return false; + } - DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); + DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); Differential diff; try { diff = new Differential(diffClient.fetchDiff()); - } catch (ArcanistUsageException e) { - logger.info("arcanist", "unable to fetch differential"); + } catch (ConduitAPIException e) { + logger.info(CONDUIT_TAG, "unable to fetch differential"); return true; } @@ -140,10 +144,10 @@ public final boolean perform(final AbstractBuild build, final Launcher lau if (uberallsConfigured) { commenter.processParentCoverage(uberalls.getParentCoverage(diff), diff.getBranch()); } else { - logger.info("uberalls", "no backend configured, skipping..."); + logger.info(UBERALLS_TAG, "no backend configured, skipping..."); } } else { - logger.info("uberalls", "no line coverage found, skipping..."); + logger.info(UBERALLS_TAG, "no line coverage found, skipping..."); } // Add in comments about the build result @@ -159,8 +163,8 @@ public final boolean perform(final AbstractBuild build, final Launcher lau String.format("Error from Harbormaster: %s", result.getString("errorMessage"))); return false; } - } catch (ArcanistUsageException e) { - logger.info("arcanist", "unable to post to harbormaster"); + } catch (ConduitAPIException e) { + logger.info(CONDUIT_TAG, "unable to post to harbormaster"); return true; } } else { @@ -196,6 +200,14 @@ public final boolean perform(final AbstractBuild build, final Launcher lau return true; } + private ConduitAPIClient getConduitClient(Job owner, Logger logger) throws ConduitAPIException { + ConduitCredentials credentials = getConduitCredentials(owner); + if (credentials == null) { + throw new ConduitAPIException("No credentials configured for conduit"); + } + return new ConduitAPIClient(credentials.getUrl(), credentials.getToken().getPlainText()); + } + private CoverageResult getUberallsCoverage(AbstractBuild build, BuildListener listener) { if (!build.getResult().isBetterOrEqualTo(Result.UNSTABLE) || !uberallsEnabled) { return null; @@ -239,8 +251,12 @@ public String getCommentFile() { return commentFile; } + private ConduitCredentials getConduitCredentials(Job owner) { + return getDescriptor().getCredentials(owner); + } + public String getConduitToken(Job owner, Logger logger) { - ConduitCredentials credentials = getDescriptor().getCredentials(owner); + ConduitCredentials credentials = getConduitCredentials(owner); if (credentials != null) { return credentials.getToken().getPlainText(); } diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorPlugin.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorPlugin.java index b57e1300..185b45a1 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorPlugin.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorPlugin.java @@ -27,8 +27,6 @@ import java.io.File; public class PhabricatorPlugin extends Plugin { - public static final String ARCANIST_PATH = "ARCANIST_PATH"; - // Diff ID (not differential ID) static final String DIFFERENTIAL_ID_FIELD = "DIFF_ID"; // Phabricator object ID (for Harbormaster) diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java new file mode 100644 index 00000000..733dcb97 --- /dev/null +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java @@ -0,0 +1,96 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.conduit; + +import net.sf.json.JSONObject; +import net.sf.json.groovy.JsonSlurper; +import org.apache.commons.httpclient.HttpStatus; +import org.apache.http.HttpResponse; +import org.apache.http.NameValuePair; +import org.apache.http.client.ClientProtocolException; +import org.apache.http.client.entity.UrlEncodedFormEntity; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.message.BasicNameValuePair; + +import java.io.IOException; +import java.io.InputStream; +import java.io.UnsupportedEncodingException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +public class ConduitAPIClient { + private static final String API_TOKEN_KEY = "api.token"; + + private final String conduitURL; + private final String conduitToken; + + public ConduitAPIClient(String conduitURL, String conduitToken) { + this.conduitURL = conduitURL; + this.conduitToken = conduitToken; + } + + /** + * Call the conduit API of Phabricator + * @param action Name of the API call + * @param data The data to send to Harbormaster + * @return The result as a JSONObject + * @throws IOException If there was a problem reading the response + * @throws ConduitAPIException If there was an error calling conduit + */ + public JSONObject perform(String action, Map data) throws IOException, ConduitAPIException { + CloseableHttpClient client = HttpClientBuilder.create().build(); + HttpUriRequest request = createRequest(action, data); + + HttpResponse response; + try { + response = client.execute(request); + } catch (ClientProtocolException e) { + throw new ConduitAPIException(e.getMessage()); + } + + InputStream responseBody = response.getEntity().getContent(); + + if (response.getStatusLine().getStatusCode() != HttpStatus.SC_OK) { + throw new ConduitAPIException(responseBody.toString(), response.getStatusLine().getStatusCode()); + } + + JsonSlurper jsonParser = new JsonSlurper(); + + return (JSONObject)jsonParser.parse(responseBody); + } + + public HttpUriRequest createRequest(String action, Map data) throws UnsupportedEncodingException { + HttpPost post = new HttpPost(String.format("%s/api/%s", conduitURL, action)); + + List params = new ArrayList(); + params.add(new BasicNameValuePair(API_TOKEN_KEY, conduitToken)); + for (Map.Entry datum : data.entrySet()) { + params.add(new BasicNameValuePair(datum.getKey(), datum.getValue())); + } + UrlEncodedFormEntity entity = new UrlEncodedFormEntity(params); + post.setEntity(entity); + return post; + } +} diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIException.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIException.java new file mode 100644 index 00000000..2d20a5c5 --- /dev/null +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIException.java @@ -0,0 +1,35 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.conduit; + +public class ConduitAPIException extends Exception { + public final int code; + + public ConduitAPIException(String message) { + super(message); + this.code = 0; + } + + public ConduitAPIException(String message, int code) { + super(message); + this.code = code; + } +} diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java index f6510757..3932702c 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java @@ -27,14 +27,13 @@ import net.sf.json.JSONNull; import net.sf.json.JSONObject; -import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; public class Differential { private JSONObject rawJSON; - public Differential(JSONObject rawJSON) throws ArcanistUsageException, IOException, InterruptedException { + public Differential(JSONObject rawJSON) { this.rawJSON = rawJSON; } diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java index bef369e4..95082c11 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -20,7 +20,6 @@ package com.uber.jenkins.phabricator.conduit; -import com.uber.jenkins.phabricator.LauncherFactory; import net.sf.json.JSONException; import net.sf.json.JSONObject; @@ -33,15 +32,11 @@ */ public class DifferentialClient { private final String diffID; - private final LauncherFactory launcher; - private final String arcPath; - private final String conduitToken; + private final ConduitAPIClient conduit; - public DifferentialClient(String diffID, LauncherFactory launcher, String conduitToken, String arcPath) { + public DifferentialClient(String diffID, ConduitAPIClient conduit) { this.diffID = diffID; - this.launcher = launcher; - this.conduitToken = conduitToken; - this.arcPath = arcPath; + this.conduit = conduit; } /** @@ -51,7 +46,7 @@ public DifferentialClient(String diffID, LauncherFactory launcher, String condui * @param silent whether or not to trigger an email * @param action phabricator comment action, e.g. 'resign', 'reject', 'none' */ - public JSONObject postComment(String revisionID, String message, boolean silent, String action) throws IOException, InterruptedException, ArcanistUsageException { + public JSONObject postComment(String revisionID, String message, boolean silent, String action) throws ConduitAPIException, IOException { Map params = new HashMap(); params.put("revision_id", revisionID); params.put("action", action); @@ -61,7 +56,7 @@ public JSONObject postComment(String revisionID, String message, boolean silent, return this.callConduit("differential.createcomment", params); } - public JSONObject fetchDiff() throws ArcanistUsageException, IOException, InterruptedException { + public JSONObject fetchDiff() throws IOException, ConduitAPIException { Map params = new HashMap(); params.put("ids", new String[]{this.diffID}); JSONObject query = this.callConduit("differential.querydiffs", params); @@ -69,7 +64,7 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr try { response = query.getJSONObject("response"); } catch (JSONException e) { - throw new ArcanistUsageException( + throw new ConduitAPIException( String.format("No 'response' object found in conduit call: (%s) %s", e.getMessage(), query.toString(2))); @@ -77,7 +72,7 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr 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", diffID, e.getMessage(), @@ -93,7 +88,7 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr * @throws IOException * @throws InterruptedException */ - public JSONObject sendHarbormasterMessage(String phid, boolean pass) throws IOException, InterruptedException, ArcanistUsageException { + public JSONObject sendHarbormasterMessage(String phid, boolean pass) throws ConduitAPIException, IOException { Map params = new HashMap(); params.put("type", pass ? "pass" : "fail"); params.put("buildTargetPHID", phid); @@ -110,27 +105,11 @@ public JSONObject sendHarbormasterMessage(String phid, boolean pass) throws IOEx * @throws InterruptedException * @throws ArcanistUsageException */ - public JSONObject postComment(String revisionID, String message) throws IOException, InterruptedException, ArcanistUsageException { + public JSONObject postComment(String revisionID, String message) throws ConduitAPIException, IOException { return postComment(revisionID, message, true, "none"); } - protected JSONObject callConduit(String methodName, Map params) throws IOException, InterruptedException, ArcanistUsageException { - ArcanistClient arc = getArcanistClient(methodName, params); - return arc.parseConduit(this.launcher.launch(), this.launcher.getStderr()); - } - - /** - * Get a new arcanist client. - * @param params parameters to pass to arcanist - * @return a new ArcanistClient - */ - protected ArcanistClient getArcanistClient(String methodName, Map params) { - return new ArcanistClient( - this.arcPath, - "call-conduit", - params, - conduitToken, - methodName - ); + protected JSONObject callConduit(String methodName, Map params) throws ConduitAPIException, IOException { + return conduit.perform(methodName, params); } } diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java new file mode 100644 index 00000000..24a7abc6 --- /dev/null +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java @@ -0,0 +1,135 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.tasks; + +import com.uber.jenkins.phabricator.LauncherFactory; +import com.uber.jenkins.phabricator.conduit.ArcanistClient; +import com.uber.jenkins.phabricator.utils.Logger; + +import java.io.IOException; +import java.io.PrintStream; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +public class ApplyPatchTask extends Task { + private final LauncherFactory starter; + private final String baseCommit; + private final String diffID; + private final PrintStream logStream; + private final String conduitToken; + private final String arcPath; + private final boolean createCommit; + private final String gitPath; + + public ApplyPatchTask(Logger logger, LauncherFactory starter, String baseCommit, + String diffID, String conduitToken, String arcPath, + String gitPath, boolean createCommit) { + super(logger); + this.starter = starter; + this.baseCommit = baseCommit; + this.diffID = diffID; + this.conduitToken = conduitToken; + this.arcPath = arcPath; + this.gitPath = gitPath; + this.createCommit = createCommit; + + this.logStream = logger.getStream(); + } + + /** + * {@inheritDoc} + */ + @Override + protected String getTag() { + return "arc-patch"; + } + + /** + * {@inheritDoc} + */ + @Override + protected void setup() { + // Do nothing + } + + /** + * {@inheritDoc} + */ + @Override + protected void execute() { + try { + int resetCode = starter.launch() + .cmds(Arrays.asList(gitPath, "reset", "--hard", baseCommit)) + .stdout(logStream) + .join(); + + if (resetCode != 0) { + info("Got non-zero exit code resetting to base commit " + baseCommit + ": " + resetCode); + } + + // Clean workspace, otherwise `arc patch` may fail + starter.launch() + .stdout(logStream) + .cmds(Arrays.asList(gitPath, "clean", "-fd", "-f")) + .join(); + + // Update submodules recursively. + starter.launch() + .stdout(logStream) + .cmds(Arrays.asList(gitPath, "submodule", "update", "--init", "--recursive")) + .join(); + + List params = new ArrayList(Arrays.asList("--nobranch", "--diff", diffID)); + if (!createCommit) { + params.add("--nocommit"); + } + + ArcanistClient arc = new ArcanistClient( + arcPath, + "patch", + null, + conduitToken, + params.toArray(new String[params.size()])); + + int result = arc.callConduit(starter.launch(), logStream); + if (result == 0) { + this.result = Result.SUCCESS; + } else { + this.result = Result.FAILURE; + } + } catch (IOException e) { + e.printStackTrace(logStream); + this.result = Result.FAILURE; + } catch (InterruptedException e) { + e.printStackTrace(logStream); + this.result = Result.FAILURE; + } + } + + /** + * {@inheritDoc} + */ + @Override + protected void tearDown() { + // Do nothing + } +} diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java index 6c5a28dc..19ec1ac6 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java @@ -20,7 +20,7 @@ package com.uber.jenkins.phabricator.tasks; -import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; +import com.uber.jenkins.phabricator.conduit.ConduitAPIException; import com.uber.jenkins.phabricator.conduit.DifferentialClient; import com.uber.jenkins.phabricator.utils.Logger; import net.sf.json.JSONNull; @@ -104,12 +104,10 @@ private JSONObject postDifferentialComment(String message, boolean silent, Strin message, silent, action); result = Result.SUCCESS; return postDifferentialCommentResult; - } catch (ArcanistUsageException e) { - info("unable to post comment"); } catch (IOException e) { throw new RuntimeException(e); - } catch (InterruptedException e) { - throw new RuntimeException(e); + } catch (ConduitAPIException e) { + info("unable to post comment"); } result = Result.FAILURE; diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java new file mode 100644 index 00000000..5f404990 --- /dev/null +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java @@ -0,0 +1,92 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.conduit; + +import com.uber.jenkins.phabricator.utils.TestUtils; +import net.sf.json.JSONObject; +import org.apache.commons.httpclient.HttpStatus; +import org.apache.http.localserver.LocalTestServer; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.UnsupportedEncodingException; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +public class ConduitAPIClientTest { + private LocalTestServer server; + private ConduitAPIClient client; + private final Map emptyParams = new HashMap(); + + @Before + public void setUp() throws Exception { + server = new LocalTestServer(null, null); + server.start(); + } + + @After + public void tearDown() throws Exception { + server.stop(); + } + + @Test(expected = ConduitAPIException.class) + public void testInvalidURI() throws Exception { + client = new ConduitAPIClient("gopher://foo", TestUtils.TEST_CONDUIT_TOKEN); + client.perform("anything", emptyParams); + } + + @Test + public void testSuccessfullFetch() throws Exception { + server.register("/api/valid", TestUtils.makeHttpHandler(HttpStatus.SC_OK, "{\"hello\": \"world\"}")); + + client = new ConduitAPIClient(getTestServerAddress(), TestUtils.TEST_CONDUIT_TOKEN); + JSONObject response = client.perform("valid", emptyParams); + assertEquals("world", response.getString("hello")); + } + + @Test(expected = ConduitAPIException.class) + public void testBadRequestErrorCode() throws Exception { + server.register("/api/foo", TestUtils.makeHttpHandler(HttpStatus.SC_BAD_REQUEST, "nothing")); + + client = new ConduitAPIClient(getTestServerAddress(), TestUtils.TEST_CONDUIT_TOKEN); + client.perform("foo", emptyParams); + } + + @Test + public void testWithParams() throws UnsupportedEncodingException { + client = new ConduitAPIClient("http://foo.bar", TestUtils.TEST_CONDUIT_TOKEN); + Map params = new HashMap(); + params.put("hello", "world"); + client.createRequest("action", params); + } + + private String getTestServerAddress() { + return String.format( + "http://%s:%s", + server.getServiceAddress().getHostName(), + server.getServiceAddress().getPort() + ); + } + +} diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java index c34da4bc..4dd91de8 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java @@ -64,7 +64,7 @@ public void testPostCommentSingleArgument() throws Exception { assertEquals(response, sentinel); } - @Test(expected = ArcanistUsageException.class) + @Test(expected = ConduitAPIException.class) public void testFetchDiffWithEmptyResponse() throws Exception { JSONObject empty = new JSONObject(); mockConduitResponse(differentialClient, empty); @@ -72,7 +72,7 @@ public void testFetchDiffWithEmptyResponse() throws Exception { differentialClient.fetchDiff(); } - @Test(expected = ArcanistUsageException.class) + @Test(expected = ConduitAPIException.class) public void testFetchDiffWithNoDiff() throws Exception { JSONObject noDiff = new JSONObject(); noDiff.put("response", null); @@ -99,7 +99,22 @@ public void testFetchDiffWithValidResponse() throws Exception { assertEquals("world", response.get("hello")); } - private void mockConduitResponse(DifferentialClient client, JSONObject response) throws InterruptedException, ArcanistUsageException, IOException { + @Test(expected = ConduitAPIException.class) + public void testFetchDiffWithJSONException() throws Exception { + JSONObject badResponse = TestUtils.getJSONFromFile(getClass(), "fetchDiffWithResponseArray"); + mockConduitResponse(differentialClient, badResponse); + + differentialClient.fetchDiff(); + } + + @Test + public void testSendHarbormasterSuccess() throws IOException, ConduitAPIException { + JSONObject empty = new JSONObject(); + mockConduitResponse(differentialClient, empty); + differentialClient.sendHarbormasterMessage(TestUtils.TEST_PHID, true); + } + + private void mockConduitResponse(DifferentialClient client, JSONObject response) throws IOException, ConduitAPIException { doReturn(response).when(client).callConduit( anyString(), anyMap() diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java new file mode 100644 index 00000000..35865b89 --- /dev/null +++ b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java @@ -0,0 +1,60 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.tasks; + +import com.uber.jenkins.phabricator.utils.TestUtils; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +import static org.junit.Assert.assertEquals; + +public class ApplyPatchTaskTest { + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + public void testApplyPatchWithValidArc() throws Exception { + ApplyPatchTask task = getTask("echo", "true"); + Task.Result result = task.run(); + assertEquals(result.SUCCESS, result); + } + + @Test + public void testApplyPatchWithInvalidArc() throws Exception { + ApplyPatchTask task = getTask("false", "echo"); + Task.Result result = task.run(); + assertEquals(result.FAILURE, result); + } + + private ApplyPatchTask getTask(String arcPath, String gitPath) throws Exception { + return new ApplyPatchTask( + TestUtils.getDefaultLogger(), + TestUtils.createLauncherFactory(j), + TestUtils.TEST_SHA, + TestUtils.TEST_DIFFERENTIAL_ID, + TestUtils.TEST_CONDUIT_TOKEN, + arcPath, + gitPath, // git path + false // createCommit + ); + } +} diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java index 1eaf5c08..0fdc4eeb 100644 --- a/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java @@ -20,7 +20,7 @@ package com.uber.jenkins.phabricator.tasks; -import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; +import com.uber.jenkins.phabricator.conduit.ConduitAPIException; import com.uber.jenkins.phabricator.conduit.DifferentialClient; import com.uber.jenkins.phabricator.utils.Logger; import com.uber.jenkins.phabricator.utils.TestUtils; @@ -50,7 +50,7 @@ public void setup() { @Test public void testPostDifferentialFailed() throws Exception { - doThrow(new ArcanistUsageException("")).when(differentialClient).postComment( + doThrow(new ConduitAPIException("")).when(differentialClient).postComment( anyString(), anyString(), anyBoolean(), diff --git a/src/test/java/com/uber/jenkins/phabricator/utils/TestUtils.java b/src/test/java/com/uber/jenkins/phabricator/utils/TestUtils.java index 175c0db6..c6bb789f 100644 --- a/src/test/java/com/uber/jenkins/phabricator/utils/TestUtils.java +++ b/src/test/java/com/uber/jenkins/phabricator/utils/TestUtils.java @@ -22,6 +22,7 @@ import com.uber.jenkins.phabricator.CodeCoverageMetrics; import com.uber.jenkins.phabricator.LauncherFactory; +import com.uber.jenkins.phabricator.conduit.ConduitAPIClient; import com.uber.jenkins.phabricator.conduit.DifferentialClient; import com.uber.jenkins.phabricator.uberalls.UberallsClient; import hudson.EnvVars; @@ -31,6 +32,12 @@ import hudson.plugins.cobertura.targets.CoverageResult; import net.sf.json.JSONObject; import net.sf.json.groovy.JsonSlurper; +import org.apache.http.HttpException; +import org.apache.http.HttpRequest; +import org.apache.http.HttpResponse; +import org.apache.http.entity.StringEntity; +import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpRequestHandler; import org.jvnet.hudson.test.JenkinsRule; import java.io.ByteArrayOutputStream; @@ -48,7 +55,8 @@ public class TestUtils { public static final String TEST_SHA = "test-sha"; public static final String TEST_DIFFERENTIAL_ID = "123"; - private static final String TEST_CONDUIT_TOKEN = "notarealtoken"; + public static final String TEST_CONDUIT_TOKEN = "notarealtoken"; + public static final String TEST_PHID = "PHID-not-real"; private static final String TEST_ARC_PATH = "echo"; public static Logger getDefaultLogger() { @@ -65,9 +73,8 @@ public static UberallsClient getDefaultUberallsClient() { } public static DifferentialClient getDefaultDifferentialClient() { - LauncherFactory factory = mock(LauncherFactory.class); - return spy(new DifferentialClient(TEST_DIFFERENTIAL_ID, factory, TEST_CONDUIT_TOKEN, - TEST_ARC_PATH)); + ConduitAPIClient client = mock(ConduitAPIClient.class); + return spy(new DifferentialClient(TEST_DIFFERENTIAL_ID, client)); } public static EnvVars getDefaultEnvVars() { @@ -121,7 +128,7 @@ public static CoverageResult getEmptyCoverageResult() { return coverageResult; } - public static JSONObject getJSONFromFile(Class klass, String filename) throws IOException { + public static JSONObject getJSONFromFile(Class klass, String filename) throws IOException { InputStream in = klass.getResourceAsStream(String.format("%s.json", filename)); return slurpFromInputStream(in); } @@ -137,4 +144,15 @@ private static void setCoverage(CoverageResult coverageResult, CoverageMetric co when(coverageResult.getCoverage(coverageMetric)).thenReturn(ratio); } } + + public static HttpRequestHandler makeHttpHandler(final int statusCode, final String body) { + return new HttpRequestHandler() { + @Override + public void handle(HttpRequest request, HttpResponse response, HttpContext context) throws HttpException, IOException { + response.setStatusCode(statusCode); + response.setEntity(new StringEntity(body)); + + } + }; + } } diff --git a/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json new file mode 100644 index 00000000..b0959685 --- /dev/null +++ b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json @@ -0,0 +1,3 @@ +{ + "response": [] +} From 5f85f41f693f1f26ad34d8eb9fdfc27b13c8fe6d Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 9 Aug 2015 18:44:53 -0700 Subject: [PATCH 2/8] Cleanup --- phabricator-plugin.iml | 8 +++---- pom.xml | 2 +- .../jenkins/phabricator/CommentBuilder.java | 2 +- .../ConduitCredentialsDescriptor.java | 2 +- .../phabricator/PhabricatorBuildWrapper.java | 22 +++++-------------- .../phabricator/PhabricatorNotifier.java | 21 +++++------------- .../phabricator/conduit/ArcanistClient.java | 18 ++------------- .../conduit/ArcanistUsageException.java | 2 +- .../phabricator/conduit/ConduitAPIClient.java | 19 +++++++++++++--- .../phabricator/conduit/Differential.java | 2 +- .../conduit/DifferentialClient.java | 8 +++---- .../phabricator/tasks/ApplyPatchTask.java | 1 - .../tasks/NonDifferentialBuildTask.java | 8 +++---- .../phabricator/tasks/PostCommentTask.java | 4 ++-- .../uber/jenkins/phabricator/tasks/Task.java | 2 +- .../jenkins/phabricator/utils/Logger.java | 2 +- .../phabricator/BuildIntegrationTest.java | 6 ++--- .../conduit/ArcanistClientTest.java | 15 +++++-------- .../conduit/ConduitAPIClientTest.java | 2 +- .../conduit/DifferentialClientTest.java | 2 +- .../tasks/PostCommentTaskTest.java | 7 +++--- .../conduit/fetchDiffResponseMissingDiff.json | 2 +- .../conduit/fetchDiffWithResponseArray.json | 2 +- .../conduit/validFetchDiffResponse.json | 2 +- 24 files changed, 66 insertions(+), 95 deletions(-) diff --git a/phabricator-plugin.iml b/phabricator-plugin.iml index 373c7a10..1bc46cdc 100644 --- a/phabricator-plugin.iml +++ b/phabricator-plugin.iml @@ -63,10 +63,10 @@ - - - - + + + + diff --git a/pom.xml b/pom.xml index 2c4fc594..71830b11 100644 --- a/pom.xml +++ b/pom.xml @@ -48,7 +48,7 @@ org.apache.httpcomponents httpclient - 4.3 + 4.4.1 diff --git a/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java b/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java index ddccfb74..f26fe6e0 100644 --- a/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java +++ b/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java @@ -27,7 +27,7 @@ import hudson.plugins.cobertura.targets.CoverageMetric; import hudson.plugins.cobertura.targets.CoverageResult; -public class CommentBuilder { +class CommentBuilder { private static final String UBERALLS_TAG = "uberalls"; private final Logger logger; private final CoverageResult currentCoverage; diff --git a/src/main/java/com/uber/jenkins/phabricator/ConduitCredentialsDescriptor.java b/src/main/java/com/uber/jenkins/phabricator/ConduitCredentialsDescriptor.java index ec1ca05d..5468df68 100644 --- a/src/main/java/com/uber/jenkins/phabricator/ConduitCredentialsDescriptor.java +++ b/src/main/java/com/uber/jenkins/phabricator/ConduitCredentialsDescriptor.java @@ -37,7 +37,7 @@ import java.util.List; public class ConduitCredentialsDescriptor { - public static List availableCredentials(Job owner) { + private static List availableCredentials(Job owner) { return CredentialsProvider.lookupCredentials( ConduitCredentials.class, owner, diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index 8091db70..9cb7683d 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -39,8 +39,6 @@ import java.io.IOException; import java.util.Arrays; -import java.util.HashMap; -import java.util.Map; public class PhabricatorBuildWrapper extends BuildWrapper { private static final String CONDUIT_TAG = "conduit"; @@ -70,8 +68,6 @@ public Environment setUp(AbstractBuild build, return this.ignoreBuild(logger, "No environment variables found?!"); } - final Map envAdditions = new HashMap(); - String diffID = environment.get(PhabricatorPlugin.DIFFERENTIAL_ID_FIELD); if (CommonUtils.isBlank(diffID)) { this.addShortText(build); @@ -114,6 +110,7 @@ public Environment setUp(AbstractBuild build, diffClient.postComment(diff.getRevisionID(false), diff.getBuildStartedMessage(environment)); } } catch (ConduitAPIException e) { + e.printStackTrace(logger.getStream()); logger.warn(CONDUIT_TAG, "Unable to apply patch"); logger.warn(CONDUIT_TAG, e.getMessage()); return null; @@ -135,16 +132,7 @@ logger, starter, baseCommit, diffID, conduitToken, getArcPath(), return null; } - return new Environment() { - @Override - public void buildEnvVars(Map env) { - // A little roundabout, but allows us to do overrides per - // how EnvVars#override works (PATH+unique=/foo/bar) - EnvVars envVars = new EnvVars(env); - envVars.putAll(envAdditions); - env.putAll(envVars); - } - }; + return new Environment(){}; } private void addShortText(final AbstractBuild build) { @@ -191,7 +179,7 @@ public boolean isShowBuildStartedMessage() { return showBuildStartedMessage; } - public String getPhabricatorURL(Job owner) { + private String getPhabricatorURL(Job owner) { ConduitCredentials credentials = getConduitCredentials(owner); if (credentials != null) { return credentials.getUrl(); @@ -199,7 +187,7 @@ public String getPhabricatorURL(Job owner) { return this.getDescriptor().getConduitURL(); } - public String getConduitToken(Job owner, Logger logger) { + private String getConduitToken(Job owner, Logger logger) { ConduitCredentials credentials = getConduitCredentials(owner); if (credentials != null) { return credentials.getToken().getPlainText(); @@ -212,7 +200,7 @@ public String getConduitToken(Job owner, Logger logger) { * Return the path to the arcanist executable * @return a string, fully-qualified or not, could just be "arc" */ - public String getArcPath() { + private String getArcPath() { final String providedPath = this.getDescriptor().getArcPath(); if (CommonUtils.isBlank(providedPath)) { return "arc"; diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java index 96c7a5c8..7b697dfa 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java @@ -106,7 +106,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau ConduitAPIClient conduitClient; try { - conduitClient = getConduitClient(build.getParent(), logger); + conduitClient = getConduitClient(build.getParent()); } catch (ConduitAPIException e) { e.printStackTrace(logger.getStream()); logger.warn(CONDUIT_TAG, e.getMessage()); @@ -118,7 +118,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau try { diff = new Differential(diffClient.fetchDiff()); } catch (ConduitAPIException e) { - logger.info(CONDUIT_TAG, "unable to fetch differential"); + logger.info(CONDUIT_TAG, "Unable to fetch differential"); return true; } @@ -158,9 +158,9 @@ public final boolean perform(final AbstractBuild build, final Launcher lau logger.info("harbormaster", "Sending build result to Harbormaster with PHID '" + phid + "', success: " + harbormasterSuccess); try { JSONObject result = diffClient.sendHarbormasterMessage(phid, harbormasterSuccess); - if (result.containsKey("errorMessage") && !(result.get("errorMessage") instanceof JSONNull)) { + if (result.containsKey("error_info") && !(result.get("error_info") instanceof JSONNull)) { logger.info("harbormaster", - String.format("Error from Harbormaster: %s", result.getString("errorMessage"))); + String.format("Error from Harbormaster: %s", result.getString("error_info"))); return false; } } catch (ConduitAPIException e) { @@ -200,7 +200,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau return true; } - private ConduitAPIClient getConduitClient(Job owner, Logger logger) throws ConduitAPIException { + private ConduitAPIClient getConduitClient(Job owner) throws ConduitAPIException { ConduitCredentials credentials = getConduitCredentials(owner); if (credentials == null) { throw new ConduitAPIException("No credentials configured for conduit"); @@ -255,16 +255,7 @@ private ConduitCredentials getConduitCredentials(Job owner) { return getDescriptor().getCredentials(owner); } - public String getConduitToken(Job owner, Logger logger) { - ConduitCredentials credentials = getConduitCredentials(owner); - if (credentials != null) { - return credentials.getToken().getPlainText(); - } - logger.warn("credentials", "No credentials configured."); - return null; - } - - public String getPhabricatorURL(Job owner) { + private String getPhabricatorURL(Job owner) { ConduitCredentials credentials = getDescriptor().getCredentials(owner); if (credentials != null) { return credentials.getUrl(); diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java index 2988f6d4..e9282ce7 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java @@ -25,25 +25,20 @@ import hudson.util.ArgumentListBuilder; import net.sf.json.JSONObject; import net.sf.json.groovy.JsonSlurper; -import org.apache.commons.io.IOUtils; import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.io.InputStream; import java.io.PrintStream; -import java.util.Map; public class ArcanistClient { private final String arcPath; private final String methodName; - private final Map params; private final String conduitToken; private final String[] arguments; - public ArcanistClient(String arcPath, String methodName, Map params, String conduitToken, String... arguments) { + public ArcanistClient(String arcPath, String methodName, String conduitToken, String... arguments) { this.arcPath = arcPath; this.methodName = methodName; - this.params = params; this.conduitToken = conduitToken; this.arguments = arguments; } @@ -59,16 +54,7 @@ private ArgumentListBuilder getConduitCommand() { } private Launcher.ProcStarter getCommand(Launcher.ProcStarter starter) throws IOException { - Launcher.ProcStarter command = starter.cmds(this.getConduitCommand()); - - if (this.params != null) { - JSONObject obj = new JSONObject(); - obj.putAll(this.params); - - InputStream jsonStream = IOUtils.toInputStream(obj.toString(), "UTF-8"); - command = command.stdin(jsonStream); - } - return command; + return starter.cmds(this.getConduitCommand()); } public int callConduit(Launcher.ProcStarter starter, PrintStream stderr) throws IOException, InterruptedException { diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistUsageException.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistUsageException.java index b2366fac..163e5fc2 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistUsageException.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistUsageException.java @@ -20,7 +20,7 @@ package com.uber.jenkins.phabricator.conduit; -public class ArcanistUsageException extends Exception { +class ArcanistUsageException extends Exception { public ArcanistUsageException(String message) { super(message); } diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java index 733dcb97..120e802b 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClient.java @@ -36,6 +36,9 @@ import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; +import java.net.MalformedURLException; +import java.net.URISyntaxException; +import java.net.URL; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -77,20 +80,30 @@ public JSONObject perform(String action, Map data) throws IOExce } JsonSlurper jsonParser = new JsonSlurper(); - return (JSONObject)jsonParser.parse(responseBody); } - public HttpUriRequest createRequest(String action, Map data) throws UnsupportedEncodingException { - HttpPost post = new HttpPost(String.format("%s/api/%s", conduitURL, action)); + public HttpUriRequest createRequest(String action, Map data) throws UnsupportedEncodingException, ConduitAPIException { + HttpPost post; + try { + post = new HttpPost( + new URL(new URL(new URL(conduitURL), "/api/"), action).toURI() + ); + } catch (MalformedURLException e) { + throw new ConduitAPIException(e.getMessage()); + } catch (URISyntaxException e) { + throw new ConduitAPIException(e.getMessage()); + } List params = new ArrayList(); params.add(new BasicNameValuePair(API_TOKEN_KEY, conduitToken)); for (Map.Entry datum : data.entrySet()) { params.add(new BasicNameValuePair(datum.getKey(), datum.getValue())); } + UrlEncodedFormEntity entity = new UrlEncodedFormEntity(params); post.setEntity(entity); + return post; } } diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java index 3932702c..f65982c5 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java @@ -31,7 +31,7 @@ import java.net.URL; public class Differential { - private JSONObject rawJSON; + private final JSONObject rawJSON; public Differential(JSONObject rawJSON) { this.rawJSON = rawJSON; diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java index 95082c11..9f506a22 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -51,21 +51,21 @@ public JSONObject postComment(String revisionID, String message, boolean silent, params.put("revision_id", revisionID); params.put("action", action); params.put("message", message); - params.put("silent", silent); + params.put("silent", Boolean.toString(silent)); return this.callConduit("differential.createcomment", params); } public JSONObject fetchDiff() throws IOException, ConduitAPIException { Map params = new HashMap(); - params.put("ids", new String[]{this.diffID}); + params.put("ids[]", diffID); JSONObject query = this.callConduit("differential.querydiffs", params); JSONObject response; try { - response = query.getJSONObject("response"); + response = query.getJSONObject("result"); } catch (JSONException e) { throw new ConduitAPIException( - String.format("No 'response' object found in conduit call: (%s) %s", + String.format("No 'result' object found in conduit call: (%s) %s", e.getMessage(), query.toString(2))); } diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java index 24a7abc6..fb3d6152 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java @@ -106,7 +106,6 @@ protected void execute() { ArcanistClient arc = new ArcanistClient( arcPath, "patch", - null, conduitToken, params.toArray(new String[params.size()])); diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/NonDifferentialBuildTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/NonDifferentialBuildTask.java index 4821b761..14848d28 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/NonDifferentialBuildTask.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/NonDifferentialBuildTask.java @@ -30,10 +30,10 @@ */ public class NonDifferentialBuildTask extends Task { - protected UberallsClient uberallsClient; - protected CodeCoverageMetrics codeCoverageMetrics; - protected boolean uberallsEnabled; - protected String commitSha; + protected final UberallsClient uberallsClient; + protected final CodeCoverageMetrics codeCoverageMetrics; + protected final boolean uberallsEnabled; + protected final String commitSha; /** * GenericBuildTask constructor. diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java index 19ec1ac6..25aa9405 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java @@ -79,10 +79,10 @@ protected void execute() { JSONObject postDifferentialCommentResult = postDifferentialComment(comment, SILENT, commentAction); if (postDifferentialCommentResult == null || - !(postDifferentialCommentResult.get("errorMessage") instanceof JSONNull)) { + !(postDifferentialCommentResult.get("error_info") instanceof JSONNull)) { if (postDifferentialCommentResult != null) { info(String.format("Got error %s with action %s", - postDifferentialCommentResult.get("errorMessage"), commentAction)); + postDifferentialCommentResult.get("error_info"), commentAction)); } info("Re-trying with action 'none'"); diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/Task.java b/src/main/java/com/uber/jenkins/phabricator/tasks/Task.java index 4b602450..9a12fd80 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/Task.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/Task.java @@ -39,7 +39,7 @@ public enum Result { } protected Result result = Result.UNKNOWN; - private Logger logger; + private final Logger logger; /** * Task constructor. diff --git a/src/main/java/com/uber/jenkins/phabricator/utils/Logger.java b/src/main/java/com/uber/jenkins/phabricator/utils/Logger.java index c8703d25..e6e23a63 100644 --- a/src/main/java/com/uber/jenkins/phabricator/utils/Logger.java +++ b/src/main/java/com/uber/jenkins/phabricator/utils/Logger.java @@ -28,7 +28,7 @@ public class Logger { private static final String LOG_FORMAT = "[%s] %s"; - private PrintStream stream; + private final PrintStream stream; /** * Logger constructor. diff --git a/src/test/java/com/uber/jenkins/phabricator/BuildIntegrationTest.java b/src/test/java/com/uber/jenkins/phabricator/BuildIntegrationTest.java index ca7b6d57..599ca042 100644 --- a/src/test/java/com/uber/jenkins/phabricator/BuildIntegrationTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/BuildIntegrationTest.java @@ -33,14 +33,14 @@ public class BuildIntegrationTest { @Rule public JenkinsRule j = new JenkinsRule(); - protected FreeStyleProject p; + FreeStyleProject p; - protected void assertSuccessfulBuild(Result result) { + void assertSuccessfulBuild(Result result) { assertTrue(result.isCompleteBuild()); assertTrue(result.isBetterOrEqualTo(Result.SUCCESS)); } - protected FreeStyleProject createProject() throws IOException { + FreeStyleProject createProject() throws IOException { return j.createFreeStyleProject(); } } diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java index 52c7676a..049f746f 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java @@ -27,9 +27,6 @@ import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; -import java.util.HashMap; -import java.util.Map; - import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -37,11 +34,9 @@ public class ArcanistClientTest { @Rule public JenkinsRule j = new JenkinsRule(); - private final Map emptyParams = new HashMap(); - @Test public void testEcho() throws Exception { - ArcanistClient client = new ArcanistClient("echo", "hello", emptyParams, null); + ArcanistClient client = new ArcanistClient("echo", "hello", null); int result = client.callConduit(getLauncher().launch(), System.err); assertEquals(result, 0); @@ -49,7 +44,7 @@ public void testEcho() throws Exception { @Test public void testEchoWithToken() throws Exception { - ArcanistClient client = new ArcanistClient("echo", "tokentest", emptyParams, "notarealtoken"); + ArcanistClient client = new ArcanistClient("echo", "tokentest", "notarealtoken"); int result = client.callConduit(getLauncher().launch(), System.err); assertEquals(result, 0); @@ -58,7 +53,7 @@ public void testEchoWithToken() throws Exception { @Test public void testParseConduit() throws Exception { String jsonString = "{\"hello\": \"world\"}"; - ArcanistClient client = new ArcanistClient("echo", jsonString, emptyParams, null); + ArcanistClient client = new ArcanistClient("echo", jsonString, null); JSONObject result = client.parseConduit(getLauncher().launch(), System.err); assertTrue(result.has("hello")); @@ -67,14 +62,14 @@ public void testParseConduit() throws Exception { @Test(expected = ArcanistUsageException.class) public void testNonZeroExitCode() throws Exception { - ArcanistClient client = new ArcanistClient("false", "", emptyParams, null); + ArcanistClient client = new ArcanistClient("false", "", null); client.parseConduit(getLauncher().launch(), System.err); } @Test(expected = JSONException.class) public void testNonJsonOutput() throws Exception { - ArcanistClient client = new ArcanistClient("echo", "not-json", emptyParams, null); + ArcanistClient client = new ArcanistClient("echo", "not-json", null); client.parseConduit(getLauncher().launch(), System.err); } diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java index 5f404990..89e48577 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/ConduitAPIClientTest.java @@ -74,7 +74,7 @@ public void testBadRequestErrorCode() throws Exception { } @Test - public void testWithParams() throws UnsupportedEncodingException { + public void testWithParams() throws UnsupportedEncodingException, ConduitAPIException { client = new ConduitAPIClient("http://foo.bar", TestUtils.TEST_CONDUIT_TOKEN); Map params = new HashMap(); params.put("hello", "world"); diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java index 4dd91de8..319dc3c2 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java @@ -75,7 +75,7 @@ public void testFetchDiffWithEmptyResponse() throws Exception { @Test(expected = ConduitAPIException.class) public void testFetchDiffWithNoDiff() throws Exception { JSONObject noDiff = new JSONObject(); - noDiff.put("response", null); + noDiff.put("result", null); mockConduitResponse(differentialClient, noDiff); differentialClient.fetchDiff(); diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java index 0fdc4eeb..cfaffa8f 100644 --- a/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java @@ -34,13 +34,12 @@ import static org.mockito.Mockito.doThrow; public class PostCommentTaskTest { - private Logger logger; private DifferentialClient differentialClient; - private String TEST_COMMENT = "They are selling like hotcakes!"; - private String TEST_COMMENT_ACTION = "none"; - private String TEST_REVISION_ID = "something"; + private final String TEST_COMMENT = "They are selling like hotcakes!"; + private final String TEST_COMMENT_ACTION = "none"; + private final String TEST_REVISION_ID = "something"; @Before public void setup() { diff --git a/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffResponseMissingDiff.json b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffResponseMissingDiff.json index 1bcefa84..7a43ca58 100644 --- a/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffResponseMissingDiff.json +++ b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffResponseMissingDiff.json @@ -1,5 +1,5 @@ { - "response": { + "result": { "not-my-diff": { "garbaj": true } diff --git a/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json index b0959685..4677642a 100644 --- a/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json +++ b/src/test/resources/com/uber/jenkins/phabricator/conduit/fetchDiffWithResponseArray.json @@ -1,3 +1,3 @@ { - "response": [] + "result": [] } diff --git a/src/test/resources/com/uber/jenkins/phabricator/conduit/validFetchDiffResponse.json b/src/test/resources/com/uber/jenkins/phabricator/conduit/validFetchDiffResponse.json index 3208af59..1d4742db 100644 --- a/src/test/resources/com/uber/jenkins/phabricator/conduit/validFetchDiffResponse.json +++ b/src/test/resources/com/uber/jenkins/phabricator/conduit/validFetchDiffResponse.json @@ -1,5 +1,5 @@ { - "response": { + "result": { "123": { "hello": "world" } From f3268e9153dcd2fa3cc55ca9d2f3c9e7dfb4317d Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 9 Aug 2015 21:30:54 -0700 Subject: [PATCH 3/8] Remove unused parseConduit --- .../jenkins/phabricator/CommentBuilder.java | 2 +- .../phabricator/conduit/ArcanistClient.java | 29 ------------------- .../conduit/ArcanistClientTest.java | 26 ----------------- .../phabricator/tasks/ApplyPatchTaskTest.java | 4 +-- 4 files changed, 3 insertions(+), 58 deletions(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java b/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java index f26fe6e0..d528cdd4 100644 --- a/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java +++ b/src/main/java/com/uber/jenkins/phabricator/CommentBuilder.java @@ -95,7 +95,7 @@ public void processParentCoverage(CodeCoverageMetrics parentCoverage, String bra } public void processBuildResult(boolean commentOnSuccess, boolean commentWithConsoleLinkOnFailure, boolean runHarbormaster) { - if (result == result.SUCCESS) { + if (result == Result.SUCCESS) { if (comment.length() == 0 && (commentOnSuccess || !runHarbormaster)) { comment.append("Build is green"); } diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java index e9282ce7..c3211e44 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/ArcanistClient.java @@ -23,10 +23,7 @@ import com.uber.jenkins.phabricator.utils.CommonUtils; import hudson.Launcher; import hudson.util.ArgumentListBuilder; -import net.sf.json.JSONObject; -import net.sf.json.groovy.JsonSlurper; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.PrintStream; @@ -61,30 +58,4 @@ public int callConduit(Launcher.ProcStarter starter, PrintStream stderr) throws Launcher.ProcStarter command = this.getCommand(starter); return command.stdout(stderr).stderr(stderr).join(); } - - public JSONObject parseConduit(Launcher.ProcStarter starter, PrintStream stderr) - throws ArcanistUsageException, IOException, InterruptedException { - Launcher.ProcStarter command = this.getCommand(starter); - ByteArrayOutputStream stdoutBuffer = new ByteArrayOutputStream(); - - int returnCode = command. - stdout(stdoutBuffer). - stderr(stderr). - join(); - - if (returnCode != 0) { - String errorMessage = stdoutBuffer.toString(); - stderr.println("[arcanist] returned non-zero exit code " + returnCode); - stderr.println("[arcanist] output: " + errorMessage); - throw new ArcanistUsageException(errorMessage); - } - - JsonSlurper jsonParser = new JsonSlurper(); - try { - return (JSONObject) jsonParser.parseText(stdoutBuffer.toString()); - } catch (net.sf.json.JSONException e) { - stderr.println("[arcanist] Unable to parse JSON from response: " + stdoutBuffer.toString()); - throw e; - } - } } diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java index 049f746f..620cd6c4 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/ArcanistClientTest.java @@ -21,14 +21,11 @@ package com.uber.jenkins.phabricator.conduit; import hudson.Launcher; -import net.sf.json.JSONException; -import net.sf.json.JSONObject; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; public class ArcanistClientTest { @Rule @@ -50,29 +47,6 @@ public void testEchoWithToken() throws Exception { assertEquals(result, 0); } - @Test - public void testParseConduit() throws Exception { - String jsonString = "{\"hello\": \"world\"}"; - ArcanistClient client = new ArcanistClient("echo", jsonString, null); - - JSONObject result = client.parseConduit(getLauncher().launch(), System.err); - assertTrue(result.has("hello")); - assertEquals(result.getString("hello"), "world"); - } - - @Test(expected = ArcanistUsageException.class) - public void testNonZeroExitCode() throws Exception { - ArcanistClient client = new ArcanistClient("false", "", null); - - client.parseConduit(getLauncher().launch(), System.err); - } - - @Test(expected = JSONException.class) - public void testNonJsonOutput() throws Exception { - ArcanistClient client = new ArcanistClient("echo", "not-json", null); - client.parseConduit(getLauncher().launch(), System.err); - } - private Launcher getLauncher() { return j.createLocalLauncher(); } diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java index 35865b89..f9358104 100644 --- a/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java @@ -35,14 +35,14 @@ public class ApplyPatchTaskTest { public void testApplyPatchWithValidArc() throws Exception { ApplyPatchTask task = getTask("echo", "true"); Task.Result result = task.run(); - assertEquals(result.SUCCESS, result); + assertEquals(Task.Result.SUCCESS, result); } @Test public void testApplyPatchWithInvalidArc() throws Exception { ApplyPatchTask task = getTask("false", "echo"); Task.Result result = task.run(); - assertEquals(result.FAILURE, result); + assertEquals(Task.Result.FAILURE, result); } private ApplyPatchTask getTask(String arcPath, String gitPath) throws Exception { From fd71a4fd994ff1307f5adbef5840f34e76f9f028 Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Mon, 10 Aug 2015 11:48:55 -0700 Subject: [PATCH 4/8] Address review feedback --- .../com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index 9cb7683d..a60151ac 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -97,9 +97,9 @@ public Environment setUp(AbstractBuild build, return null; } - DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); Differential diff; try { + DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); diff = new Differential(diffClient.fetchDiff()); diff.decorate(build, this.getPhabricatorURL(build.getParent())); From dbe92a0f39cf236661770770c1dd075e5f610bc3 Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Mon, 10 Aug 2015 12:42:57 -0700 Subject: [PATCH 5/8] Clarify conduit exception message --- .../uber/jenkins/phabricator/conduit/DifferentialClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java index 9f506a22..4a9b603e 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -73,7 +73,7 @@ public JSONObject fetchDiff() throws IOException, ConduitAPIException { return response.getJSONObject(diffID); } catch (JSONException e) { throw new ConduitAPIException( - String.format("Unable to find '%s' key in response: (%s) %s", + String.format("Unable to find '%s' key in 'result': (%s) %s", diffID, e.getMessage(), response.toString(2))); From bf661b8eb6c49dfe4fb92ab98c1437cfd025af0f Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Mon, 10 Aug 2015 12:49:44 -0700 Subject: [PATCH 6/8] Address more review feedback --- .../phabricator/PhabricatorBuildWrapper.java | 6 +++--- .../phabricator/tasks/ApplyPatchTask.java | 20 ++++++++----------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index a60151ac..adb8c87f 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -184,7 +184,7 @@ private String getPhabricatorURL(Job owner) { if (credentials != null) { return credentials.getUrl(); } - return this.getDescriptor().getConduitURL(); + return getDescriptor().getConduitURL(); } private String getConduitToken(Job owner, Logger logger) { @@ -193,7 +193,7 @@ private String getConduitToken(Job owner, Logger logger) { return credentials.getToken().getPlainText(); } logger.warn("credentials", "No credentials configured. Falling back to deprecated configuration."); - return this.getDescriptor().getConduitToken(); + return getDescriptor().getConduitToken(); } /** @@ -201,7 +201,7 @@ private String getConduitToken(Job owner, Logger logger) { * @return a string, fully-qualified or not, could just be "arc" */ private String getArcPath() { - final String providedPath = this.getDescriptor().getArcPath(); + final String providedPath = getDescriptor().getArcPath(); if (CommonUtils.isBlank(providedPath)) { return "arc"; } diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java index fb3d6152..d364a7bc 100644 --- a/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java +++ b/src/main/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTask.java @@ -77,13 +77,13 @@ protected void setup() { @Override protected void execute() { try { - int resetCode = starter.launch() + int exitCode = starter.launch() .cmds(Arrays.asList(gitPath, "reset", "--hard", baseCommit)) .stdout(logStream) .join(); - if (resetCode != 0) { - info("Got non-zero exit code resetting to base commit " + baseCommit + ": " + resetCode); + if (exitCode != 0) { + info("Got non-zero exit code resetting to base commit " + baseCommit + ": " + exitCode); } // Clean workspace, otherwise `arc patch` may fail @@ -98,23 +98,19 @@ protected void execute() { .cmds(Arrays.asList(gitPath, "submodule", "update", "--init", "--recursive")) .join(); - List params = new ArrayList(Arrays.asList("--nobranch", "--diff", diffID)); + List arcPatchParams = new ArrayList(Arrays.asList("--nobranch", "--diff", diffID)); if (!createCommit) { - params.add("--nocommit"); + arcPatchParams.add("--nocommit"); } ArcanistClient arc = new ArcanistClient( arcPath, "patch", conduitToken, - params.toArray(new String[params.size()])); + arcPatchParams.toArray(new String[arcPatchParams.size()])); - int result = arc.callConduit(starter.launch(), logStream); - if (result == 0) { - this.result = Result.SUCCESS; - } else { - this.result = Result.FAILURE; - } + exitCode = arc.callConduit(starter.launch(), logStream); + this.result = exitCode == 0 ? Result.SUCCESS : Result.FAILURE; } catch (IOException e) { e.printStackTrace(logStream); this.result = Result.FAILURE; From 27b43a1d8d55ccd80cc352ac8fd2549b237dc6fa Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Mon, 10 Aug 2015 13:25:43 -0700 Subject: [PATCH 7/8] Add test; remove trove4j --- phabricator-plugin.iml | 53 +------------------ pom.xml | 7 --- .../jenkins/phabricator/utils/LoggerTest.java | 9 ++++ 3 files changed, 10 insertions(+), 59 deletions(-) diff --git a/phabricator-plugin.iml b/phabricator-plugin.iml index 1bc46cdc..52c23a28 100644 --- a/phabricator-plugin.iml +++ b/phabricator-plugin.iml @@ -12,57 +12,6 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -121,7 +70,6 @@ - @@ -223,6 +171,7 @@ + diff --git a/pom.xml b/pom.xml index 71830b11..69ed23e0 100644 --- a/pom.xml +++ b/pom.xml @@ -57,13 +57,6 @@ 1.9.6 - - net.sf.trove4j - trove4j - 3.0.3 - jar - - org.jenkins-ci.plugins credentials diff --git a/src/test/java/com/uber/jenkins/phabricator/utils/LoggerTest.java b/src/test/java/com/uber/jenkins/phabricator/utils/LoggerTest.java index 3f7131bf..43a23530 100644 --- a/src/test/java/com/uber/jenkins/phabricator/utils/LoggerTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/utils/LoggerTest.java @@ -49,4 +49,13 @@ public void testInfo() { logger.info(tag, message); assertEquals("[phabricator-jenkins] This is a great plugin!\n", byteArrayOutputStream.toString()); } + + @Test + public void testWarn() { + String tag = "a-softer-world"; + String message = "This is a great comic"; + + logger.info(tag, message); + assertEquals("[a-softer-world] This is a great comic\n", byteArrayOutputStream.toString()); + } } From e881c467ccdaf30a6832f2e0a59677648594462c Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Mon, 10 Aug 2015 14:07:39 -0700 Subject: [PATCH 8/8] Add another test for git failure --- .../uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java index f9358104..a0947460 100644 --- a/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/tasks/ApplyPatchTaskTest.java @@ -45,6 +45,12 @@ public void testApplyPatchWithInvalidArc() throws Exception { assertEquals(Task.Result.FAILURE, result); } + @Test + public void testBothGitAndArcFailing() throws Exception { + ApplyPatchTask task = getTask("false", "false"); + assertEquals(Task.Result.FAILURE, task.run()); + } + private ApplyPatchTask getTask(String arcPath, String gitPath) throws Exception { return new ApplyPatchTask( TestUtils.getDefaultLogger(),