fix: --variable silently ignored on Create release steps that do not deploy

octopus-agent/src/main/java/octopus/teamcity/agent/cli/CommandHelper.java

@@ -6,8 +6,12 @@
 
 import java.io.File;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import jetbrains.buildServer.agent.BuildProgressLogger;
 import jetbrains.buildServer.agent.impl.artifacts.ArtifactsCollection;
@@ -20,12 +24,55 @@
 
 public class CommandHelper {
 
-  static final String deployReleaseAdditionalArgumentsToBeIgnored =
-      "-e,--environment,--tenant,--tenant-tag,--deploy-at,--deploy-at-expiry,--variable,--update-variables,--skip,--guided-failure,--force-package-download,--deployment-target,--exclude-deployment-target,--deployment-freeze-name,--deployment-freeze-override-reason";
-  static final String runbookRunAdditionalArgumentsToBeIgnored =
-      "-p,--project,-n,--name,-e,--environment,--tenant,--tenant-tag,--snapshot,--space";
-  static final String createReleaseAdditionalArgumentsToBeIgnored =
-      "-c,-r,-x,--channel,--git-ref,--git-commit,--package,--git-resource,--release-notes,--release-notes-file,--ignore-existing,--ignore-channel-rules,--custom-field";
+  static final Set<String> deployReleaseAdditionalArgumentsToBeIgnored =
+      argumentSet(
+          "-e",
+          "--environment",
+          "--tenant",
+          "--tenant-tag",
+          "--deploy-at",
+          "--deploy-at-expiry",
+          "--variable",
+          // -v is --variable on "release deploy" but --version on "release create", so it has to be
+          // stripped from the create command or a prompted variable is silently used as the release
+          // number. The release number has its own field, so losing -v as a create-side alias for
+          // --version is the cheaper side of the ambiguity.
+          "-v",
+          "--update-variables",
+          "--skip",
+          "--guided-failure",
+          "--force-package-download",
+          "--deployment-target",
+          "--exclude-deployment-target",
+          "--deployment-freeze-name",
+          "--deployment-freeze-override-reason");
+  static final Set<String> runbookRunAdditionalArgumentsToBeIgnored =
+      argumentSet(
+          "-p",
+          "--project",
+          "-n",
+          "--name",
+          "-e",
+          "--environment",
+          "--tenant",
+          "--tenant-tag",
+          "--snapshot",
+          "--space");
+  static final Set<String> createReleaseAdditionalArgumentsToBeIgnored =
+      argumentSet(
+          "-c",
+          "-r",
+          "-x",
+          "--channel",
+          "--git-ref",
+          "--git-commit",
+          "--package",
+          "--git-resource",
+          "--release-notes",
+          "--release-notes-file",
+          "--ignore-existing",
+          "--ignore-channel-rules",
+          "--custom-field");
   static final String defaultSpace = "Default";
 
   public static String[] deployRelease(
@@ -427,17 +474,49 @@ public static String[] wait(Map<String, String> params, String taskId) {
     return commands.toArray(new String[0]);
   }
 
-  public static List<String> sanitizeCommandArgs(List<String> args, String argsToBeIgnore) {
-    List<String> result = new ArrayList<>();
+  /**
+   * Removes the arguments in {@code argsToBeIgnored} - along with their values - from a list of
+   * user-supplied additional command line arguments.
+   *
+   * <p>The "Additional command line arguments" field is shared by every command a step runs, so
+   * each command has to drop the arguments that belong to the other. Matching is exact: a substring
+   * match would strip {@code --var} because {@code --variable} is on the list. A value is only
+   * consumed when the following token is not itself an argument, so switches that take no value
+   * (such as {@code --update-variables}) no longer swallow the argument that follows them.
+   *
+   * <p>The consequence is that a value which legitimately starts with {@code -} is left behind as a
+   * stray positional argument. That is rare, and preferable to silently dropping a real argument.
+   */
+  public static List<String> sanitizeCommandArgs(
+      final List<String> args, final Set<String> argsToBeIgnored) {
+    final List<String> result = new ArrayList<>();
     int i = 0;
     while (i < args.size()) {
-      if (argsToBeIgnore.contains(args.get(i))) {
-        i += 2; // skip this element and the next element
-      } else {
-        result.add(args.get(i));
-        i++;
+      final String arg = args.get(i);
+      i++;
+      if (!isIgnored(arg, argsToBeIgnored)) {
+        result.add(arg);
+      } else if (!hasInlineValue(arg) && i < args.size() && !isArgumentName(args.get(i))) {
+        i++; // the following token is this argument's value
       }
     }
     return result;
   }
+
+  private static Set<String> argumentSet(final String... args) {
+    return Collections.unmodifiableSet(new LinkedHashSet<>(Arrays.asList(args)));
+  }
+
+  private static boolean isIgnored(final String arg, final Set<String> argsToBeIgnored) {
+    final int equals = arg.indexOf('=');
+    return argsToBeIgnored.contains(equals > 0 ? arg.substring(0, equals) : arg);
+  }
+
+  private static boolean hasInlineValue(final String arg) {
+    return arg.indexOf('=') > 0;
+  }
+
+  private static boolean isArgumentName(final String token) {
+    return token.startsWith("-");
+  }
 }

octopus-agent/src/test/java/octopus/teamcity/agent/cli/CommandHelperTest.java

@@ -9,8 +9,10 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import jetbrains.buildServer.agent.AgentRunningBuild;
 import jetbrains.buildServer.agent.BuildRunnerContext;
@@ -357,12 +359,16 @@ void loginCommandUsesApiKeyForNonOidcSource() {
     assertThat(command).doesNotContain("--service-account-id", "--id-token");
   }
 
+  private static Set<String> ignoring(String... args) {
+    return new LinkedHashSet<>(Arrays.asList(args));
+  }
+
   @Test
   void sanitizeCommandExtraArgsRemovesForbiddenArgAndValue() {
     List<String> args =
         Arrays.asList("project", "MyProject", "channel", "Release", "version", "1.0.0");
 
-    List<String> result = CommandHelper.sanitizeCommandArgs(args, "channel");
+    List<String> result = CommandHelper.sanitizeCommandArgs(args, ignoring("channel"));
 
     assertThat(result).containsExactly("project", "MyProject", "version", "1.0.0");
   }
@@ -373,7 +379,7 @@ void sanitizeCommandExtraArgsRemovesMultipleForbiddenArgs() {
         Arrays.asList(
             "project", "MyProject", "channel", "Release", "tenant", "TenantA", "version", "1.0.0");
 
-    List<String> result = CommandHelper.sanitizeCommandArgs(args, "channel, tenant");
+    List<String> result = CommandHelper.sanitizeCommandArgs(args, ignoring("channel", "tenant"));
 
     assertThat(result).containsExactly("project", "MyProject", "version", "1.0.0");
   }
@@ -382,8 +388,83 @@ void sanitizeCommandExtraArgsRemovesMultipleForbiddenArgs() {
   void sanitizeCommandExtraArgsKeepsArgsWhenNoForbiddenPresent() {
     List<String> args = Arrays.asList("project", "MyProject", "version", "1.0.0");
 
-    List<String> result = CommandHelper.sanitizeCommandArgs(args, "channel");
+    List<String> result = CommandHelper.sanitizeCommandArgs(args, ignoring("channel"));
 
     assertThat(result).containsExactly("project", "MyProject", "version", "1.0.0");
   }
+
+  @Test
+  void sanitizeCommandExtraArgsDoesNotConsumeArgumentAfterAValuelessSwitch() {
+    List<String> args = Arrays.asList("--ignore-existing", "--variable", "ImageTag:1.5.2");
+
+    List<String> result =
+        CommandHelper.sanitizeCommandArgs(
+            args, CommandHelper.createReleaseAdditionalArgumentsToBeIgnored);
+
+    assertThat(result).containsExactly("--variable", "ImageTag:1.5.2");
+  }
+
+  @Test
+  void sanitizeCommandExtraArgsDoesNotLeaveAStrayValueBehind() {
+    List<String> args = Arrays.asList("--update-variables", "--channel", "Beta");
+
+    List<String> result =
+        CommandHelper.sanitizeCommandArgs(
+            args, CommandHelper.deployReleaseAdditionalArgumentsToBeIgnored);
+
+    assertThat(result).containsExactly("--channel", "Beta");
+  }
+
+  @Test
+  void sanitizeCommandExtraArgsMatchesArgumentNamesExactly() {
+    List<String> args = Arrays.asList("--var", "Something", "--e", "SomethingElse");
+
+    List<String> result =
+        CommandHelper.sanitizeCommandArgs(
+            args, CommandHelper.deployReleaseAdditionalArgumentsToBeIgnored);
+
+    assertThat(result).containsExactly("--var", "Something", "--e", "SomethingElse");
+  }
+
+  @Test
+  void sanitizeCommandExtraArgsRemovesForbiddenArgWithAnInlineValue() {
+    List<String> args = Arrays.asList("--variable=ImageTag:1.5.2", "--channel", "Beta");
+
+    List<String> result =
+        CommandHelper.sanitizeCommandArgs(
+            args, CommandHelper.deployReleaseAdditionalArgumentsToBeIgnored);
+
+    assertThat(result).containsExactly("--channel", "Beta");
+  }
+
+  @Test
+  void createReleaseWithDeployToForwardsTheShortPromptedVariableFormToTheDeployCommand() {
+    // -v is --variable on "release deploy" but --version on "release create". Left on the create
+    // command the CLI rejects the release number: "The release number 'ImageTag:1.5.2' does not
+    // appear to be a valid version number".
+    Map<String, String> params = new HashMap<>();
+    final OctopusConstants constants = OctopusConstants.Instance;
+    params.put(constants.getProjectNameKey(), "MyProject");
+    params.put(constants.getDeployToKey(), "Dev");
+    params.put(constants.getCommandLineArgumentsKey(), "-v ImageTag:1.5.2");
+
+    assertThat(CommandHelper.createRelease(params).buildCommand())
+        .doesNotContain("-v", "ImageTag:1.5.2");
+    assertThat(CommandHelper.deployRelease(params, "1.0.0"))
+        .containsSequence("-v", "ImageTag:1.5.2");
+  }
+
+  @Test
+  void createReleaseWithDeployToForwardsPromptedVariablesToTheDeployCommand() {
+    Map<String, String> params = new HashMap<>();
+    final OctopusConstants constants = OctopusConstants.Instance;
+    params.put(constants.getProjectNameKey(), "MyProject");
+    params.put(constants.getDeployToKey(), "Dev");
+    params.put(constants.getCommandLineArgumentsKey(), "--variable ImageTag:1.5.2");
+
+    assertThat(CommandHelper.createRelease(params).buildCommand())
+        .doesNotContain("--variable", "ImageTag:1.5.2");
+    assertThat(CommandHelper.deployRelease(params, "1.0.0"))
+        .containsSequence("--variable", "ImageTag:1.5.2");
+  }
 }

octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java

@@ -2,6 +2,7 @@
 
 import static octopus.teamcity.server.PropertiesValidator.checkCredentialsUnlessUsingConnection;
 import static octopus.teamcity.server.PropertiesValidator.checkNotEmpty;
+import static octopus.teamcity.server.PropertiesValidator.checkPromptedVariablesOnlyWhenDeploying;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -58,6 +59,7 @@ public Collection<InvalidProperty> process(@Nullable final Map<String, String> p
 
         checkCredentialsUnlessUsingConnection(p, c, result);
         checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result);
+        checkPromptedVariablesOnlyWhenDeploying(p, c, result);
 
         if (result.isEmpty()) {
           ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p);

octopus-server/src/main/java/octopus/teamcity/server/PropertiesValidator.java

@@ -17,6 +17,7 @@
 
 import java.util.Collection;
 import java.util.Map;
+import java.util.regex.Pattern;
 
 import jetbrains.buildServer.serverSide.InvalidProperty;
 import jetbrains.buildServer.util.StringUtil;
@@ -26,6 +27,19 @@
 /** Shared validation helpers for run type {@code PropertiesProcessor}s. */
 public final class PropertiesValidator {
 
+  private static final Pattern VARIABLE_ARGUMENT =
+      Pattern.compile("(?:^|\\s)--variable(?:[\\s=]|$)");
+
+  /**
+   * The legacy {@code octo} CLI takes the deploy target as {@code --deployTo} on {@code
+   * create-release}, and its option parser is case insensitive and accepts {@code -}, {@code --}
+   * and {@code /} prefixes. A step that supplies the target that way rather than through the
+   * "Deploy to" field does deploy, so {@code --variable} takes effect and must not be reported as
+   * useless.
+   */
+  private static final Pattern DEPLOY_TO_ARGUMENT =
+      Pattern.compile("(?:^|\\s)(?:--?|/)deployto(?:[\\s=]|$)", Pattern.CASE_INSENSITIVE);
+
   private PropertiesValidator() {}
 
   /**
@@ -57,4 +71,40 @@ public static void checkCredentialsUnlessUsingConnection(
       checkNotEmpty(properties, constants.getApiKey(), "API key must be specified", result);
     }
   }
+
+  /**
+   * Rejects {@code --variable} in the additional command line arguments of a step that only creates
+   * a release. Prompted variables are supplied when a release is deployed, so the Octopus CLI
+   * accepts the argument on a create-only command and then ignores it, leaving the deployment to
+   * fail later with "Please provide a variable for the prompted value".
+   *
+   * <p>A step deploys when "Deploy to" is set, and also when the additional arguments carry the
+   * legacy {@code --deployTo}; only a step that does neither is reported. Which CLI will run is an
+   * agent-side decision, so this cannot be narrowed further here - on the new CLI {@code
+   * --deployTo} is not a {@code release create} argument at all and the step fails with {@code
+   * unknown flag}, which is loud enough to diagnose without help from this check.
+   */
+  public static void checkPromptedVariablesOnlyWhenDeploying(
+      @NotNull final Map<String, String> properties,
+      @NotNull final OctopusConstants constants,
+      @NotNull final Collection<InvalidProperty> result) {
+    final String additionalArguments = properties.get(constants.getCommandLineArgumentsKey());
+    if (StringUtil.isEmptyOrSpaces(additionalArguments)) {
+      return;
+    }
+    if (!VARIABLE_ARGUMENT.matcher(additionalArguments).find()) {
+      return;
+    }
+    if (DEPLOY_TO_ARGUMENT.matcher(additionalArguments).find()) {
+      return;
+    }
+    if (StringUtil.isEmptyOrSpaces(properties.get(constants.getDeployToKey()))) {
+      result.add(
+          new InvalidProperty(
+              constants.getCommandLineArgumentsKey(),
+              "--variable supplies prompted variables to a deployment, so it has no effect on a "
+                  + "step that only creates a release. Set 'Deploy to' on this step, or move "
+                  + "--variable to an 'OctopusDeploy: Deploy release' step."));
+    }
+  }
 }

octopus-server/src/main/resources/buildServerResources/forms/editOctopusCreateReleaseForm.jsp

@@ -141,7 +141,8 @@
     <td>
       <props:textProperty name="${keys.commandLineArgumentsKey}" className="longField"/>
       <span class="error" id="error_${keys.commandLineArgumentsKey}"></span>
-      <span class="smallNote">Additional arguments to be passed to <a href="https://g.octopushq.com/OctoExeCreateRelease">Octopus CLI</a></span>
+      <span class="smallNote">Additional arguments to be passed to <a href="https://g.octopushq.com/OctoExeCreateRelease">Octopus CLI</a>.
+        <code>--variable</code> supplies prompted variables to a <em>deployment</em>, so it only takes effect when <strong>Deploy to</strong> is also set on this step.</span>
     </td>
   </tr>
 </l:settingsGroup>