Review comments #3
Conversation
| dsl: '1.0.2' | ||
| namespace: test | ||
| name: run-example | ||
| name: container-cleanup-default |
There was a problem hiding this comment.
Each workflow file should have a different id
| DockerClientImpl.getInstance( | ||
| defaultConfig, | ||
| new ApacheDockerHttpClient.Builder().dockerHost(defaultConfig.getDockerHost()).build()); | ||
| app = WorkflowApplication.builder().build(); |
There was a problem hiding this comment.
Its better to create the application just once (in order this to work, all definition ids should be different)
| } catch (Exception e) { | ||
| throw new RuntimeException("Workflow execution failed", e); | ||
| } | ||
| Map<String, Object> result = |
There was a problem hiding this comment.
if you use join rather than get, there is not exception to catch
| this.createContainerCmd = createContainerCmd; | ||
| this.configuration = configuration; | ||
| } | ||
| interface ContainerPropertySetter { |
There was a problem hiding this comment.
ContainerPropertySetter is now a interface which implementations add things to the container command
Since implemenation are very likely to contain expressions, they will be evaluated with the help of the context
| DockerClientImpl.getInstance( | ||
| DEFAULT_CONFIG, | ||
| new ApacheDockerHttpClient.Builder().dockerHost(DEFAULT_CONFIG.getDockerHost()).build()); | ||
| private static class DockerClientHolder { |
There was a problem hiding this comment.
This defers initialization of the docker client till runtime, before this change this was invoked during parsing time
| private final WorkflowDefinition definition; | ||
| private final Collection<ContainerPropertySetter> propertySetters; | ||
| private final Optional<WorkflowValueResolver<Duration>> timeout; | ||
| private final ContainerCleanupPolicy policy; |
There was a problem hiding this comment.
The only information from the Container task that is needed at runtime (once the ContainerPropertySetter implementations are changed) are the policy and container image name
| if (exit == null || exit == 0) { | ||
| return input; | ||
| } else { | ||
| throw mapExitCode(exit); |
There was a problem hiding this comment.
I changed this because the exception generated from mapExitCode was capture but the catch exception block in the former implementation, rendering it useless
| return input; | ||
| } | ||
| Integer exit = executeContainer(workflowContext, taskContext, input); | ||
| if (exit == null || exit == 0) { |
There was a problem hiding this comment.
Its unclear to me if exit == null is equivalent to 0, but I respected the previous implementation
| throw new IllegalStateException("Container creation failed: empty ID"); | ||
| } | ||
| dockerClient.startContainerCmd(id).exec(); | ||
| DockerClientHolder.dockerClient.startContainerCmd(id).exec(); |
There was a problem hiding this comment.
Container command is now created during runtime, not parsing time
fjtirado
force-pushed
the
container_review_comments
branch
from
aa21ee9 to
b38faab
Compare
| CommandPropertySetter(CreateContainerCmd createContainerCmd, Container configuration) { | ||
| super(createContainerCmd, configuration); | ||
| CommandPropertySetter(WorkflowDefinition definition, Container configuration) { | ||
| String commandName = configuration.getCommand(); |
There was a problem hiding this comment.
During parsing time (when the constructor is invoked) we create the filter to resolve the command
| WorkflowContext workflowContext, | ||
| TaskContext taskContext, | ||
| WorkflowModel model) { | ||
| command |
There was a problem hiding this comment.
During runtime, we evaluate the filter and then set container command with that info
fjtirado
force-pushed
the
container_review_comments
branch
2 times, most recently
from
8af9d2f to
7175b71
Compare
Signed-off-by: fjtirado <[email protected]>
fjtirado
force-pushed
the
container_review_comments
branch
from
7175b71 to
ec25d5b
Compare
Signed-off-by: fjtirado <[email protected]>
2 participants
Requested changes for serverlessworkflow#942
Comments are inlined