refactor: migrate CommandsWrapper commands to dynamic properties

fix: render before command with options in CommandsWrapper (#7496)
refactor: return only command when no interpreter and no beforeCommands (#7452)
refactor: remove rendering from Docker (#7439)
rendering is done in the CommandsWrapper so no need to render again in the task runner
refactor: introduce render in commands wrapper for property string (#7430)
fix: enable rendering of commands properties inside CommandsWrapper (#7381)
refactor: migrate commands to Property in TaskCommands and CommandsWrapper
implement beforeCommand and interpreter

Author: mgabelle

core/src/main/java/io/kestra/core/models/tasks/runners/ScriptService.java

@@ -2,6 +2,7 @@
 
 import com.google.common.collect.ImmutableMap;
 import io.kestra.core.exceptions.IllegalVariableEvaluationException;
+import io.kestra.core.models.property.Property;
 import io.kestra.core.runners.RunContext;
 import io.kestra.core.utils.ListUtils;
 import io.kestra.core.utils.Slugify;
@@ -93,6 +94,18 @@ public static List<String> replaceInternalStorage(
 
     }
 
+    public static List<String> replaceInternalStorage(
+        RunContext runContext,
+        Map<String, Object> additionalVars,
+        Property<List<String>> commands,
+        boolean replaceWithRelativePath
+    ) throws IOException, IllegalVariableEvaluationException {
+        return commands == null ? Collections.emptyList() :
+            runContext.render(commands).asList(String.class, additionalVars).stream()
+                .map(throwFunction(c -> ScriptService.replaceInternalStorage(runContext, c, replaceWithRelativePath)))
+                .toList();
+    }
+
     public static List<String> replaceInternalStorage(
         RunContext runContext,
         List<String> commands

core/src/main/java/io/kestra/core/models/tasks/runners/TaskCommands.java

@@ -1,5 +1,8 @@
 package io.kestra.core.models.tasks.runners;
 
+import io.kestra.core.models.property.Property;
+import lombok.With;
+
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -19,7 +22,11 @@ public interface TaskCommands {
 
     AbstractLogConsumer getLogConsumer();
 
-    List<String> getCommands();
+    Property<List<String>> getInterpreter();
+
+    Property<List<String>> getBeforeCommands();
+
+    Property<List<String>> getCommands();
 
     Map<String, Object> getAdditionalVars();
 

core/src/main/java/io/kestra/core/models/tasks/runners/TaskRunner.java

@@ -5,6 +5,7 @@
 import io.kestra.core.exceptions.IllegalVariableEvaluationException;
 import io.kestra.core.models.Plugin;
 import io.kestra.core.models.WorkerJobLifecycle;
+import io.kestra.core.models.property.Property;
 import io.kestra.core.runners.RunContext;
 import io.kestra.plugin.core.runner.Process;
 import jakarta.validation.constraints.NotBlank;
@@ -16,11 +17,11 @@
 import lombok.experimental.SuperBuilder;
 import org.apache.commons.lang3.SystemUtils;
 
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.io.IOException;
+import java.util.*;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
 
 import static io.kestra.core.utils.WindowsUtils.windowsToUnixPath;
 

core/src/main/java/io/kestra/plugin/core/runner/Process.java

@@ -1,7 +1,9 @@
 package io.kestra.plugin.core.runner;
 
+import io.kestra.core.exceptions.IllegalVariableEvaluationException;
 import io.kestra.core.models.annotations.Example;
 import io.kestra.core.models.annotations.Plugin;
+import io.kestra.core.models.property.Property;
 import io.kestra.core.models.tasks.runners.*;
 import io.kestra.core.runners.RunContext;
 import io.micronaut.core.annotation.Introspected;
@@ -14,6 +16,7 @@
 import org.slf4j.Logger;
 
 import java.io.BufferedReader;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
@@ -133,11 +136,14 @@ public TaskRunnerResult<TaskRunnerDetailResult> run(RunContext runContext, TaskC
         environment.putAll(this.env(runContext, taskCommands));
 
         processBuilder.directory(taskCommands.getWorkingDirectory().toFile());
-        processBuilder.command(taskCommands.getCommands());
+
+        List<String> renderedCommands = runContext.render(taskCommands.getCommands()).asList(String.class);
+
+        processBuilder.command(renderedCommands);
 
         java.lang.Process process = processBuilder.start();
         long pid = process.pid();
-        logger.debug("Starting command with pid {} [{}]", pid, String.join(" ", taskCommands.getCommands()));
+        logger.debug("Starting command with pid {} [{}]", pid, String.join(" ", renderedCommands));
 
         LogRunnable stdOutRunnable = new LogRunnable(process.getInputStream(), defaultLogConsumer, false);
         LogRunnable stdErrRunnable = new LogRunnable(process.getErrorStream(), defaultLogConsumer, true);

core/src/test/java/io/kestra/core/models/tasks/runners/TaskRunnerTest.java

@@ -1,6 +1,7 @@
 package io.kestra.core.models.tasks.runners;
 
 import io.kestra.core.exceptions.IllegalVariableEvaluationException;
+import io.kestra.core.models.property.Property;
 import io.kestra.core.runners.RunContext;
 import io.kestra.core.runners.RunContextFactory;
 import io.micronaut.context.ApplicationContext;
@@ -149,7 +150,17 @@ public AbstractLogConsumer getLogConsumer() {
         }
 
         @Override
-        public List<String> getCommands() {
+        public Property<List<String>> getInterpreter() {
+            return null;
+        }
+
+        @Override
+        public Property<List<String>> getBeforeCommands() {
+            return null;
+        }
+
+        @Override
+        public Property<List<String>> getCommands() {
             return null;
         }
 

script/src/main/java/io/kestra/plugin/scripts/exec/AbstractExecScript.java

@@ -4,6 +4,8 @@
 import io.kestra.core.models.annotations.PluginProperty;
 import io.kestra.core.models.property.Property;
 import io.kestra.core.models.tasks.*;
+import io.kestra.core.models.tasks.runners.RemoteRunnerInterface;
+import io.kestra.core.models.tasks.runners.ScriptService;
 import io.kestra.core.models.tasks.runners.TargetOS;
 import io.kestra.core.models.tasks.runners.TaskRunner;
 import io.kestra.core.runners.RunContext;
@@ -166,9 +168,14 @@ protected CommandsWrapper commands(RunContext runContext) throws IllegalVariable
             .withOutputFiles(runContext.render(this.getOutputFiles()).asList(String.class))
             .withEnableOutputDirectory(runContext.render(this.getOutputDirectory()).as(Boolean.class).orElse(null))
             .withTimeout(runContext.render(this.getTimeout()).as(Duration.class).orElse(null))
-            .withTargetOS(runContext.render(this.getTargetOS()).as(TargetOS.class).orElseThrow());
+            .withTargetOS(runContext.render(this.getTargetOS()).as(TargetOS.class).orElseThrow())
+            .withFailFast(runContext.render(this.getFailFast()).as(Boolean.class).orElse(false));
     }
 
+    /**
+     * Rendering of beforeCommands will be done in the CommandsWrapper to give access to the workingDir variable
+     */
+    @Deprecated(since = "0.22")
     protected List<String> getBeforeCommandsWithOptions(RunContext runContext) throws IllegalVariableEvaluationException {
         return mayAddExitOnErrorCommands(runContext.render(this.getBeforeCommands()).asList(String.class), runContext);
     }

script/src/main/java/io/kestra/plugin/scripts/exec/scripts/runners/CommandsWrapper.java

@@ -1,6 +1,7 @@
 package io.kestra.plugin.scripts.exec.scripts.runners;
 
 import io.kestra.core.exceptions.IllegalVariableEvaluationException;
+import io.kestra.core.models.property.Property;
 import io.kestra.core.models.tasks.RunnableTaskException;
 import io.kestra.core.models.tasks.runners.DefaultLogConsumer;
 import io.kestra.core.models.tasks.runners.*;
@@ -20,15 +21,16 @@
 import lombok.AllArgsConstructor;
 import lombok.Getter;
 import lombok.With;
+import org.apache.commons.lang3.SystemUtils;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
 import java.nio.file.Path;
 import java.time.Duration;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
+
+import static io.kestra.core.utils.Rethrow.throwFunction;
 
 @AllArgsConstructor
 @Getter
@@ -42,7 +44,19 @@ public class CommandsWrapper implements TaskCommands {
     private Map<String, Object> additionalVars;
 
     @With
-    private List<String> commands;
+    private Property<List<String>> interpreter;
+
+    @With
+    private Property<List<String>> beforeCommands;
+
+    @With
+    private Property<List<String>> commands;
+
+    @With
+    private boolean beforeCommandsWithOptions;
+
+    @With
+    private boolean failFast;
 
     private Map<String, String> env;
 
@@ -96,7 +110,11 @@ public CommandsWrapper withEnv(Map<String, String> envs) {
             workingDirectory,
             getOutputDirectory(),
             additionalVars,
+            interpreter,
+            beforeCommands,
             commands,
+            beforeCommandsWithOptions,
+            failFast,
             envs,
             logConsumer,
             runnerType,
@@ -155,7 +173,21 @@ public <T extends TaskRunnerDetailResult> ScriptOutput run() throws Exception {
         RunContextInitializer initializer = ((DefaultRunContext) runContext).getApplicationContext().getBean(RunContextInitializer.class);
 
         RunContext taskRunnerRunContext = initializer.forPlugin(((DefaultRunContext) runContext).clone(), realTaskRunner);
-        this.commands = this.render(runContext, commands);
+
+        List<String> renderedCommands = this.renderCommands(runContext, commands);
+        List<String> renderedBeforeCommands = this.renderCommands(runContext, beforeCommands);
+        List<String> renderedInterpreter = this.renderCommands(runContext, interpreter);
+
+        List<String> finalCommands = renderedBeforeCommands.isEmpty() && renderedInterpreter.isEmpty() ?
+            renderedCommands :
+            ScriptService.scriptCommands(
+                renderedInterpreter,
+                this.isBeforeCommandsWithOptions() ? getBeforeCommandsWithOptions(renderedBeforeCommands) :  renderedBeforeCommands,
+                renderedCommands,
+                Optional.ofNullable(targetOS).orElse(TargetOS.AUTO)
+            );
+
+        this.commands = Property.of(finalCommands);
 
         ScriptOutput.ScriptOutputBuilder scriptOutputBuilder = ScriptOutput.builder()
             .warningOnStdErr(this.warningOnStdErr);
@@ -244,7 +276,18 @@ public String render(RunContext runContext, String command, List<String> interna
         );
     }
 
-    public List<String> render(RunContext runContext, List<String> commands) throws IllegalVariableEvaluationException, IOException {
+    public String render(RunContext runContext, Property<String> command) throws IllegalVariableEvaluationException, IOException {
+        TaskRunner<?> taskRunner = this.getTaskRunner();
+        if (command == null) {
+            return null;
+        }
+
+        return runContext.render(command).as(String.class, taskRunner.additionalVars(runContext, this))
+            .map(throwFunction(c -> ScriptService.replaceInternalStorage(runContext, c, taskRunner instanceof RemoteRunnerInterface)))
+            .orElse(null);
+    }
+
+    public List<String> renderCommands(RunContext runContext, Property<List<String>> commands) throws IllegalVariableEvaluationException, IOException {
         TaskRunner<?> taskRunner = this.getTaskRunner();
         return ScriptService.replaceInternalStorage(
             this.runContext,
@@ -253,4 +296,31 @@ public List<String> render(RunContext runContext, List<String> commands) throws
             taskRunner instanceof RemoteRunnerInterface
         );
     }
+
+    protected List<String> getBeforeCommandsWithOptions(List<String> beforeCommands) throws IllegalVariableEvaluationException {
+        if (!this.isFailFast()) {
+            return beforeCommands;
+        }
+
+        if (beforeCommands == null || beforeCommands.isEmpty()) {
+            return getExitOnErrorCommands();
+        }
+
+        ArrayList<String> newCommands = new ArrayList<>(beforeCommands.size() + 1);
+        newCommands.addAll(getExitOnErrorCommands());
+        newCommands.addAll(beforeCommands);
+        return newCommands;
+    }
+
+    protected List<String> getExitOnErrorCommands() {
+        TargetOS os = this.getTargetOS();
+
+        // If targetOS is Windows OR targetOS is AUTO && current system is windows and we use process as a runner.(TLDR will run on windows)
+        if (os == TargetOS.WINDOWS ||
+            (os == TargetOS.AUTO && SystemUtils.IS_OS_WINDOWS && this.getTaskRunner() instanceof Process)) {
+            return List.of("");
+        }
+        // errexit option may be unsupported by non-shell interpreter.
+        return List.of("set -e");
+    }
 }

script/src/main/java/io/kestra/plugin/scripts/runner/docker/Docker.java

@@ -429,11 +429,13 @@ public TaskRunnerResult<DockerTaskRunnerDetailResult> run(RunContext runContext,
             // start container
             dockerClient.startContainerCmd(exec.getId()).exec();
 
+            List<String> renderedCommands = runContext.render(taskCommands.getCommands()).asList(String.class);
+
             if (logger.isDebugEnabled()) {
                 logger.debug(
                     "Starting command with container id {} [{}]",
                     exec.getId(),
-                    String.join(" ", taskCommands.getCommands())
+                    String.join(" ", renderedCommands)
                 );
             }
 
@@ -641,7 +643,7 @@ private DockerClient dockerClient(RunContext runContext, String image, String ho
         return DockerService.client(dockerClientConfig);
     }
 
-    private CreateContainerCmd configure(TaskCommands taskCommands, DockerClient dockerClient, RunContext runContext, Map<String, Object> additionalVars) throws IllegalVariableEvaluationException {
+    private CreateContainerCmd configure(TaskCommands taskCommands, DockerClient dockerClient, RunContext runContext, Map<String, Object> additionalVars) throws IllegalVariableEvaluationException, IOException {
         Optional<Boolean> volumeEnabledConfig = runContext.pluginConfiguration(VOLUME_ENABLED_CONFIG);
         if (volumeEnabledConfig.isEmpty()) {
             // check the legacy property and emit a warning if used
@@ -779,7 +781,7 @@ private CreateContainerCmd configure(TaskCommands taskCommands, DockerClient doc
 
         return container
             .withHostConfig(hostConfig)
-            .withCmd(taskCommands.getCommands())
+            .withCmd(runContext.render(taskCommands.getCommands()).asList(String.class))
             .withAttachStderr(true)
             .withAttachStdout(true);
     }