Fix SharedParameters Issue and Tidy-up

There was an issue in which we assumed that the phabricator settings were always set, this is obviously incorrect so check that they exist before attempting to add them to shared parameters for the agent to use.

Also gone through and tidied up a few loose ends highlighted by SonarQube.

Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorAgentLogger.java

@@ -4,18 +4,20 @@ import jetbrains.buildServer.log.Loggers;
 
 public final class PhabricatorAgentLogger implements PhabricatorPluginLogger {
 
+    private final String LOGGING_PREFIX_TEMPLATE = "Phabricator Plugin - %s";
+
     @Override
     public void info(String message) {
-        Loggers.AGENT.info(String.format("Phabricator Plugin - %s", message));
+        Loggers.AGENT.info(String.format(LOGGING_PREFIX_TEMPLATE, message));
     }
 
     @Override
     public void warn(String message, Exception e) {
-        Loggers.AGENT.warn(String.format("Phabricator Plugin - %s", message), e);
+        Loggers.AGENT.warn(String.format(LOGGING_PREFIX_TEMPLATE, message), e);
     }
 
     @Override
     public void error(String message, Exception e) {
-        Loggers.AGENT.error(String.format("Phabricator Plugin - %s", message), e);
+        Loggers.AGENT.error(String.format(LOGGING_PREFIX_TEMPLATE, message), e);
     }
 }

Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginConfig.java

@@ -47,7 +47,7 @@ public class PhabricatorPluginConfig {
     public void setParameters(Map<String, String> parameters) {
         params = parameters;
 
-        logger.info(String.format("Looking for parameters"));
+        logger.info("Looking for parameters");
         for (String param : params.keySet()) {
             if (!isNullOrEmpty(param)) {
                 logger.info(String.format("Found %s", param));
@@ -62,26 +62,35 @@ public class PhabricatorPluginConfig {
                         logger.warn(String.format("Failed to parse phabricator URL: %s",
                                 params.get(Constants.PHABRICATOR_URL_SETTING)), e);
                     }
+                    break;
                 case Constants.PHABRICATOR_CONDUIT_TOKEN_SETTING:
                     logger.info("Found Phabricator Conduit Token");
                     conduitToken = params.get(Constants.PHABRICATOR_CONDUIT_TOKEN_SETTING);
-
+                    break;
                 case Constants.BRANCH_NAME:
                     logger.info(String.format("Found branch name: %s", params.get(Constants.BRANCH_NAME)));
                     branchName = params.get(Constants.BRANCH_NAME);
+                    break;
                 case Constants.BUILD_ID:
                     logger.info(String.format("Found build id: %s", params.get(Constants.BUILD_ID)));
                     buildId = params.get(Constants.BUILD_ID);
+                    break;
                 case Constants.DIFF_ID:
                     logger.info(String.format("Found diff ID: %s", params.get(Constants.DIFF_ID)));
                     diffId = params.get(Constants.DIFF_ID);
+                    break;
                 case Constants.HARBORMASTER_PHID:
                     logger.info(String.format("Found harbormaster target PHID: %s",
                             params.get(Constants.HARBORMASTER_PHID)));
                     harbormasterPHID = params.get(Constants.HARBORMASTER_PHID);
+                    break;
                 case Constants.REVISION_ID:
                     logger.info(String.format("Found revision ID: %s", params.get(Constants.REVISION_ID)));
                     revisionId = params.get(Constants.REVISION_ID);
+                    break;
+                default:
+                    // Another parameter which we do not care about
+                    break;
                 }
             }
         }
@@ -110,16 +119,6 @@ public class PhabricatorPluginConfig {
 
     private URL parsePhabricatorURL(String input) throws MalformedURLException {
         URL inputURL = new URL(input);
-        String scheme = inputURL.getProtocol();
-        if (scheme == null)
-            scheme = "http";
-        int port = inputURL.getPort();
-        if (port == -1) {
-            if (scheme == "https")
-                port = 443;
-            else
-                port = 80;
-        }
         return inputURL;
     }
 

Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorServerLogger.java

@@ -4,18 +4,20 @@ import jetbrains.buildServer.log.Loggers;
 
 public final class PhabricatorServerLogger implements PhabricatorPluginLogger {
 
+    private final String LOGGING_PREFIX_TEMPLATE = "Phabricator Plugin - %s";
+
     @Override
     public void info(String message) {
-        Loggers.SERVER.info(String.format("Phabricator Plugin - %s", message));
+        Loggers.SERVER.info(String.format(LOGGING_PREFIX_TEMPLATE, message));
     }
 
     @Override
     public void warn(String message, Exception e) {
-        Loggers.SERVER.warn(String.format("Phabricator Plugin - %s", message), e);
+        Loggers.SERVER.warn(String.format(LOGGING_PREFIX_TEMPLATE, message), e);
     }
 
     @Override
     public void error(String message, Exception e) {
-        Loggers.SERVER.error(String.format("Phabricator Plugin - %s", message), e);
+        Loggers.SERVER.error(String.format(LOGGING_PREFIX_TEMPLATE, message), e);
     }
 }

Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/BuildTracker.java

@@ -76,7 +76,7 @@ public class BuildTracker implements Runnable {
 
             logger.info(String.format("Build %s finished: %s", build.getBuildNumber(), build.getBuildStatus()));
             String harbormasterBuildStatus = parseTeamCityBuildStatus(build.getBuildStatus());
-            NotifyHarbormaster(harbormasterBuildStatus);
+            notifyHarbormaster(harbormasterBuildStatus);
         }
     }
 
@@ -84,7 +84,7 @@ public class BuildTracker implements Runnable {
      * Compose and dispatch an API call to harbormaster to notify of the build
      * status
      */
-    private void NotifyHarbormaster(String buildStatus) {
+    private void notifyHarbormaster(String buildStatus) {
         URL phabricatorURL = phabricatorConfig.getPhabricatorURL();
 
         try (CloseableHttpClient client = HttpClients.createDefault()) {
@@ -110,7 +110,7 @@ public class BuildTracker implements Runnable {
         }
     }
 
-    private String parseTeamCityBuildStatus(Status buildFinishedStatus){
+    private String parseTeamCityBuildStatus(Status buildFinishedStatus) {
         if (buildFinishedStatus.isSuccessful()) {
             return "pass";
         }

Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorBuildStartContextProcessor.java

@@ -13,10 +13,24 @@ public class PhabricatorBuildStartContextProcessor implements BuildStartContextP
         // agent
         Map<String, String> parameters = context.getBuild().getBuildOwnParameters();
 
+        if (parameters.containsKey(Constants.BRANCH_NAME)) {
             context.addSharedParameter(Constants.BRANCH_NAME, parameters.get(Constants.BRANCH_NAME));
+        }
+
+        if (parameters.containsKey(Constants.BUILD_ID)) {
             context.addSharedParameter(Constants.BUILD_ID, parameters.get(Constants.BUILD_ID));
+        }
+
+        if (parameters.containsKey(Constants.DIFF_ID)) {
             context.addSharedParameter(Constants.DIFF_ID, parameters.get(Constants.DIFF_ID));
+        }
+
+        if (parameters.containsKey(Constants.HARBORMASTER_PHID)) {
             context.addSharedParameter(Constants.HARBORMASTER_PHID, parameters.get(Constants.HARBORMASTER_PHID));
+        }
+
+        if (parameters.containsKey(Constants.REVISION_ID)) {
             context.addSharedParameter(Constants.REVISION_ID, parameters.get(Constants.REVISION_ID));
         }
+    }
 }

Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginBuildFeature.java

@@ -48,9 +48,10 @@ public class PhabricatorPluginBuildFeature extends BuildFeature {
     public String describeParameters(@NotNull final Map<String, String> params) {
         String url = "";
 
-        for (String key : params.keySet()) {
+        for (Map.Entry<String, String> entry : params.entrySet()) {
+            String key = entry.getKey();
             if (key.equals(Constants.PHABRICATOR_URL_SETTING)) {
-                url = params.get(key);
+                url = entry.getValue();
             }
         }