feat(plugins): filled in scaffolding #11
Conversation
There was a problem hiding this comment.
I probably know all of this, deep in my brain, but I was initially confused by what's happening in this PR so let me describe what I'm seeing and you can either confirm or deny. It seems like we're deviating from what the API is today and defining a new API for building stages in a plugin. I just want to make sure I'm on the same page.
Users who build a plugin stage will implement the Stage interface, which has an execute method. At runtime, every one of these stages will be registered with the StageResolver after being turned into an ApiStageDefinitionBuilder.
The ApiStageDefinitionBuilder will build a taskGraph with a single ApiTask which introspects the type required by the execute method, converts the context to the appropriate type and calls the execute method, passing in the desired type.
Am I correct in assuming that we're moving away from stages being composed of multiple task classes and opting for a single API execute which does everything? It seems that moving in this direction will cause us to lose the benefits of having multiple tasks like being able to use RetryableTask to retry a subset of operations within a stage or inject tasks depending the presence or absense of context values. From a UX perspective this is definitely simpler, so that's a plus!
| StageInput stageInput = | ||
| new StageInput( | ||
| objectMapper.convertValue( | ||
| stage.getContext(), GenericTypeResolver.resolveType(type, typeVariableMap))); |
There was a problem hiding this comment.
So, the idea here is that I can have a StageInput class which looks like
public class MyPluginStageInput extends StageInput {
public String foo;
}
and use it as my stage input like so
public execute(MyPluginStageInput input) {}
Is this correct?
There was a problem hiding this comment.
One would create a new class like:
public class MyPluginStageInput {
}
and then in their new stage do:
public class MyStage implements Stage<MyPluginStageInput> {
@Override
public <MyPluginStageInput> StageOutput execute(StageInput<MyPluginStageInput> stageInput) {
| Class[] cArg = new Class[1]; | ||
| cArg[0] = StageInput.class; | ||
| Method method = apiStage.getClass().getMethod("execute", cArg); | ||
| Type type = ResolvableType.forMethodParameter(method, 0).getGeneric().getType(); |
There was a problem hiding this comment.
Small nit: imight be a bit more descriptive to use inputType instead of type for this variable name.
| status = ExecutionStatus.TERMINAL; | ||
| } | ||
|
|
||
| return TaskResult.ofStatus(status); |
There was a problem hiding this comment.
Don't we want to throw the outputs of the stage back into the context here? This would allow us to use any outputs from the stage in downstream stages.
TaskResult.builder(status).context(outputs.getOutputs()).outputs(outputs.getOutputs()).build();
| ExecutionStatus status; | ||
| try { | ||
| Class[] cArg = new Class[1]; | ||
| cArg[0] = StageInput.class; |
There was a problem hiding this comment.
You can do Arrays.asList to clean this up.
| @@ -0,0 +1,61 @@ | |||
| /* | |||
| * Copyright 2019 Netflix, Inc. | |||
|
|
||
| then: | ||
| results.getStatus() == ExecutionStatus.SUCCEEDED | ||
| results.getContext()["hello"] == "world" |
There was a problem hiding this comment.
The tiniest of nits - you just do results.context.hello since this is Groovy.
| import com.google.common.annotations.Beta; | ||
|
|
||
| @Beta | ||
| public interface Stage<T> { |
There was a problem hiding this comment.
I'm not a fan of duplicated class name. It hurts readability and increases complexity by having to determine which type of Stage object you're working with.
No description provided.