Cleanup

uber-archive · Aug 10, 2015 · 6dc74f5 · 6dc74f5
1 parent d7cecc0
commit 6dc74f5
Show file tree

Hide file tree

Showing 23 changed files with 62 additions and 91 deletions.
diff --git a/phabricator-plugin.iml b/phabricator-plugin.iml
@@ -63,10 +63,10 @@
     <orderEntry type="library" scope="TEST" name="Maven: org.apache.maven.wagon:wagon-http-shared:2.4" level="project" />
     <orderEntry type="library" scope="TEST" name="Maven: org.apache.jackrabbit:jackrabbit-webdav:2.5.2" level="project" />
     <orderEntry type="library" scope="TEST" name="Maven: org.jenkins-ci:SECURITY-144-compat:1.0" level="project" />
-    <orderEntry type="library" name="Maven: org.apache.httpcomponents:httpclient:4.3" level="project" />
-    <orderEntry type="library" name="Maven: org.apache.httpcomponents:httpcore:4.3" level="project" />
-    <orderEntry type="library" name="Maven: commons-logging:commons-logging:1.1.3" level="project" />
-    <orderEntry type="library" name="Maven: commons-codec:commons-codec:1.6" level="project" />
+    <orderEntry type="library" name="Maven: org.apache.httpcomponents:httpclient:4.4.1" level="project" />
+    <orderEntry type="library" name="Maven: org.apache.httpcomponents:httpcore:4.4.1" level="project" />
+    <orderEntry type="library" name="Maven: commons-logging:commons-logging:1.2" level="project" />
+    <orderEntry type="library" name="Maven: commons-codec:commons-codec:1.9" level="project" />
     <orderEntry type="library" name="Maven: org.jenkins-ci.plugins:cobertura:1.9.6" level="project" />
     <orderEntry type="library" name="Maven: org.jenkins-ci.main:maven-plugin:1.424" level="project" />
     <orderEntry type="library" name="Maven: org.jenkins-ci.main.maven:maven-agent:1.2" level="project" />

diff --git a/pom.xml b/pom.xml
@@ -48,7 +48,7 @@
     <dependency>
       <groupId>org.apache.httpcomponents</groupId>
       <artifactId>httpclient</artifactId>
-      <version>4.3</version>
+      <version>4.4.1</version>
     </dependency>
 
     <dependency>

diff --git 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
@@ -37,7 +37,7 @@
 import java.util.List;
 
 public class ConduitCredentialsDescriptor {
-    public static List<ConduitCredentials> availableCredentials(Job owner) {
+    private static List<ConduitCredentials> 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
@@ -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<String, String> envAdditions = new HashMap<String, String>();
-
         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<String, String> 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,15 +179,15 @@ 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();
         }
         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
@@ -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;
         }
 
@@ -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
@@ -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<String, String> params;
     private final String conduitToken;
     private final String[] arguments;
 
-    public ArcanistClient(String arcPath, String methodName, Map<String, String> 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
@@ -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
@@ -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<String, String> data) throws IOExce
         }
 
         JsonSlurper jsonParser = new JsonSlurper();
-
         return (JSONObject)jsonParser.parse(responseBody);
     }
 
-    public HttpUriRequest createRequest(String action, Map<String, String> data) throws UnsupportedEncodingException {
-        HttpPost post = new HttpPost(String.format("%s/api/%s", conduitURL, action));
+    public HttpUriRequest createRequest(String action, Map<String, String> 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<NameValuePair> params = new ArrayList<NameValuePair>();
         params.add(new BasicNameValuePair(API_TOKEN_KEY, conduitToken));
         for (Map.Entry<String, String> 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
@@ -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
@@ -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<String, String>();
-        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
@@ -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
@@ -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/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
@@ -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
@@ -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();
     }
 }