From a97efb1597b786c34500a8b36c85f010ae9cd82e Mon Sep 17 00:00:00 2001 From: Steven Cooney Date: Tue, 4 Jun 2019 11:04:25 +0100 Subject: [PATCH] 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. --- .../phabricator/PhabricatorAgentLogger.java | 8 ++++--- .../phabricator/PhabricatorPluginConfig.java | 23 +++++++++--------- .../phabricator/PhabricatorServerLogger.java | 8 ++++--- .../teamcity/phabricator/BuildTracker.java | 6 ++--- ...PhabricatorBuildStartContextProcessor.java | 24 +++++++++++++++---- .../PhabricatorPluginBuildFeature.java | 5 ++-- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorAgentLogger.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorAgentLogger.java index cba064c..8e54cb1 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorAgentLogger.java +++ b/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); } } diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginConfig.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginConfig.java index 35a6b49..9ddc12c 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginConfig.java +++ b/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 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; } diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorServerLogger.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorServerLogger.java index 188742e..c2421e2 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-common/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorServerLogger.java +++ b/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); } } diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/BuildTracker.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/BuildTracker.java index 4d2e250..c6f1507 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/BuildTracker.java +++ b/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"; } diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorBuildStartContextProcessor.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorBuildStartContextProcessor.java index f70731f..305a5ec 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorBuildStartContextProcessor.java +++ b/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 parameters = context.getBuild().getBuildOwnParameters(); - context.addSharedParameter(Constants.BRANCH_NAME, parameters.get(Constants.BRANCH_NAME)); - context.addSharedParameter(Constants.BUILD_ID, parameters.get(Constants.BUILD_ID)); - context.addSharedParameter(Constants.DIFF_ID, parameters.get(Constants.DIFF_ID)); - context.addSharedParameter(Constants.HARBORMASTER_PHID, parameters.get(Constants.HARBORMASTER_PHID)); - context.addSharedParameter(Constants.REVISION_ID, parameters.get(Constants.REVISION_ID)); + 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)); + } } } diff --git a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginBuildFeature.java b/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginBuildFeature.java index 27442b6..402bb6f 100644 --- a/Teamcity-Phabricator-Plugin/phabricator-plugin-server/src/main/java/uk/xlab/teamcity/phabricator/PhabricatorPluginBuildFeature.java +++ b/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 params) { String url = ""; - for (String key : params.keySet()) { + for (Map.Entry entry : params.entrySet()) { + String key = entry.getKey(); if (key.equals(Constants.PHABRICATOR_URL_SETTING)) { - url = params.get(key); + url = entry.getValue(); } }