Skip to content

Commit

Permalink
refactor(TaskResult): Add a TaskResultBuilder and use it everywhere
Browse files Browse the repository at this point in the history
There are a significant number of times when a variable named 'outputs'
is passed to the 'context' field of TaskResult instead of the 'outputs'
field. I don't know if these are bugs, but it should be less error-prone
this way.
  • Loading branch information
plumpy committed Apr 30, 2019
1 parent cbea43c commit 4fc492e
Show file tree
Hide file tree
Showing 199 changed files with 459 additions and 516 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class DeleteProjectStage implements StageDefinitionBuilder {
"notification.type": "deleteproject"
]

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs)
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).build()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class UpsertProjectStage implements StageDefinitionBuilder {
"notification.type": "upsertproject"
]

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs)
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).build()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,20 @@ class DeleteApplicationTask extends AbstractFront50Task {
front50Service.deletePermission(application.name)
} catch (RetrofitError re) {
if (re.response?.status == 404) {
return new TaskResult(ExecutionStatus.SUCCEEDED, [:], [:])
return TaskResult.SUCCEEDED
}
log.error("Could not create or update application permission", re)
return new TaskResult(ExecutionStatus.TERMINAL, [:], outputs)
return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
}
}
} catch (RetrofitError e) {
if (e.response?.status == 404) {
return new TaskResult(ExecutionStatus.SUCCEEDED, [:], [:])
return TaskResult.SUCCEEDED
}
log.error("Could not create or update application permission", e)
return new TaskResult(ExecutionStatus.TERMINAL, [:], outputs)
return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
}
return new TaskResult(ExecutionStatus.SUCCEEDED, [:], outputs)
return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build()
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class UpsertApplicationTask extends AbstractFront50Task implements ApplicationNa
}

outputs.newState = application ?: [:]
return new TaskResult(ExecutionStatus.SUCCEEDED, [:], outputs)
return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build()
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class VerifyApplicationHasNoDependenciesTask implements Task {
} catch (RetrofitError e) {
if (!e.response) {
def exception = [operation: stage.tasks[-1].name, reason: e.message]
return new TaskResult(ExecutionStatus.TERMINAL, [exception: exception])
return TaskResult.builder(ExecutionStatus.TERMINAL).context([exception: exception]).build()
} else if (e.response && e.response.status && e.response.status != 404) {
def resp = e.response
def exception = [statusCode: resp.status, operation: stage.tasks[-1].name, url: resp.url, reason: resp.reason]
Expand All @@ -70,22 +70,22 @@ class VerifyApplicationHasNoDependenciesTask implements Task {
} catch (ignored) {
}

return new TaskResult(ExecutionStatus.TERMINAL, [exception: exception])
return TaskResult.builder(ExecutionStatus.TERMINAL).context([exception: exception]).build()
}
}

if (!existingDependencyTypes) {
return new TaskResult(ExecutionStatus.SUCCEEDED)
return TaskResult.ofStatus(ExecutionStatus.SUCCEEDED)
}

return new TaskResult(ExecutionStatus.TERMINAL, [exception: [
return TaskResult.builder(ExecutionStatus.TERMINAL).context([exception: [
details: [
error: "Application has outstanding dependencies",
errors: existingDependencyTypes.collect {
"Application is associated with one or more ${it}" as String
}
]
]])
]]).build()
}

protected Map getOortResult(String applicationName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class BakeStage implements StageDefinitionBuilder {
return deploymentDetails
}
]
new TaskResult(ExecutionStatus.SUCCEEDED, [:], globalContext)
TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(globalContext).build()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,6 @@ class CompletedBakeTask implements Task {
results.imageName = bake.imageName ?: bake.amiName
}

new TaskResult(ExecutionStatus.SUCCEEDED, results)
TaskResult.builder(ExecutionStatus.SUCCEEDED).context(results).build()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ class CreateBakeTask implements RetryableTask {
}
}

new TaskResult(ExecutionStatus.SUCCEEDED, stageOutputs)
TaskResult.builder(ExecutionStatus.SUCCEEDED).context(stageOutputs).build()
} catch (RetrofitError e) {
if (e.response?.status && e.response.status == 404) {
try {
Expand All @@ -138,7 +138,7 @@ class CreateBakeTask implements RetryableTask {
// do nothing
}

return new TaskResult(ExecutionStatus.RUNNING)
return TaskResult.ofStatus(ExecutionStatus.RUNNING)
}
throw e
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ class MonitorBakeTask implements OverridableTimeoutRetryableTask {
if (isCanceled(newStatus.state) && previousStatus.state == BakeStatus.State.PENDING) {
log.info("Original bake was 'canceled', re-baking (executionId: ${stage.execution.id}, previousStatus: ${previousStatus.state})")
def rebakeResult = createBakeTask.execute(stage)
return new TaskResult(ExecutionStatus.RUNNING, rebakeResult.context, rebakeResult.outputs)
return TaskResult.builder(ExecutionStatus.RUNNING).context(rebakeResult.context).outputs(rebakeResult.outputs).build()
}

new TaskResult(mapStatus(newStatus), [status: newStatus])
TaskResult.builder(mapStatus(newStatus)).context([status: newStatus]).build()
} catch (RetrofitError e) {
if (e.response?.status == 404) {
return new TaskResult(ExecutionStatus.RUNNING)
return TaskResult.ofStatus(ExecutionStatus.RUNNING)
}
throw e
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public TaskResult execute(@Nonnull Stage stage) {
Map<String, Object> outputs = new HashMap<>();
outputs.put("artifacts", Collections.singleton(result));

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs, outputs);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).outputs(outputs).build();
}

@Data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class MonitorBakeTaskSpec extends Specification {
)
}
task.createBakeTask = Mock(CreateBakeTask) {
1 * execute(_) >> { return new TaskResult(ExecutionStatus.SUCCEEDED, [stage: 1], [global: 2]) }
1 * execute(_) >> { return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([stage: 1]).outputs([global: 2]).build() }
}

when:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,6 @@ class CaptureSourceServerGroupCapacityTask implements Task {
}
}

return new TaskResult(ExecutionStatus.SUCCEEDED, stageOutputs)
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(stageOutputs).build()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class ModifyAwsScalingProcessStage extends TargetServerGroupLinearStageSupport {
isComplete = suspendedProcesses?.intersect(stageData.processes) == stageData.processes
}

return isComplete ? new TaskResult(ExecutionStatus.SUCCEEDED) : new TaskResult(ExecutionStatus.RUNNING)
return isComplete ? TaskResult.ofStatus(ExecutionStatus.SUCCEEDED) : TaskResult.ofStatus(ExecutionStatus.RUNNING)
}

@CompileDynamic
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ public TaskResult execute(@Nonnull Stage stage) {
.getAutoscalingPolicy()
.get("mode");
return AutoscalingMode.valueOf(autoscalingMode) == data.getMode() ?
new TaskResult(ExecutionStatus.SUCCEEDED) :
new TaskResult(ExecutionStatus.RUNNING);
TaskResult.SUCCEEDED :
TaskResult.RUNNING;
}

private TargetServerGroup getTargetGroupForLocation(StageData data, String location) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,18 @@ public TaskResult execute(Stage stage) {
results.put("interestingHealthProviderNames", interestingHealthProviderNames);
}

return new TaskResult(ExecutionStatus.SUCCEEDED, results);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(results).build();
}

if (stage.getContext().containsKey("interestingHealthProviderNames")) {
// should not override any stage-specified health providers
return new TaskResult(ExecutionStatus.SUCCEEDED);
return TaskResult.SUCCEEDED;
}

String platformSpecificHealthProviderName = healthProviderNamesByPlatform.get(getCloudProvider(stage));
if (platformSpecificHealthProviderName == null) {
log.warn("Unable to determine platform health provider for unknown cloud provider '{}'", getCloudProvider(stage));
return new TaskResult(ExecutionStatus.SUCCEEDED);
return TaskResult.SUCCEEDED;
}

try {
Expand All @@ -112,23 +112,23 @@ public TaskResult execute(Stage stage) {

if (front50Service == null) {
log.warn("Unable to determine health providers for an application without front50 enabled.");
return new TaskResult(ExecutionStatus.SUCCEEDED);
return TaskResult.SUCCEEDED;
}

Application application = front50Service.get(applicationName);

if (application.platformHealthOnly == Boolean.TRUE && application.platformHealthOnlyShowOverride != Boolean.TRUE) {
// if `platformHealthOnlyShowOverride` is true, the expectation is that `interestingHealthProviderNames` will
// be included in the request if it's desired ... and that it should NOT be automatically added.
return new TaskResult(ExecutionStatus.SUCCEEDED, Collections.singletonMap(
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(Collections.singletonMap(
"interestingHealthProviderNames", Collections.singletonList(platformSpecificHealthProviderName)
));
)).build();
}
} catch (Exception e) {
log.error("Unable to determine platform health provider (executionId: {}, stageId: {})", stage.getExecution().getId(), stage.getId(), e);
}

return new TaskResult(ExecutionStatus.SUCCEEDED);
return TaskResult.SUCCEEDED;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class MonitorKatoTask implements RetryableTask {
TaskResult execute(Stage stage) {
TaskId taskId = stage.context."kato.last.task.id" as TaskId
if (!taskId) {
return new TaskResult(ExecutionStatus.SUCCEEDED)
return TaskResult.ofStatus(ExecutionStatus.SUCCEEDED)
}

Task katoTask
Expand Down Expand Up @@ -101,7 +101,7 @@ class MonitorKatoTask implements RetryableTask {

registry.counter("monitorKatoTask.taskNotFound.retry").increment()
ctx['kato.task.notFoundRetryCount'] = ((stage.context."kato.task.notFoundRetryCount" as Integer) ?: 0) + 1
return new TaskResult(ExecutionStatus.RUNNING, ctx)
return TaskResult.builder(ExecutionStatus.RUNNING).context(ctx).build()
} else {
throw re
}
Expand Down Expand Up @@ -152,7 +152,7 @@ class MonitorKatoTask implements RetryableTask {

}

new TaskResult(status, outputs)
TaskResult.builder(status).context(outputs).build()
}

private static ExecutionStatus katoStatusToTaskStatus(Task katoTask, boolean katoResultExpected) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,6 @@ public TaskResult execute(@Nonnull Stage stage) {
.put("deploy.account.name", credentials)
.build();

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ public TaskResult execute(@Nonnull Stage stage) {

if (match == null) {
outputs.put("exception", "No artifact matching " + expectedArtifact + " found among " + priorArtifacts);
return new TaskResult(ExecutionStatus.TERMINAL, new HashMap<>(), outputs);
return TaskResult.builder(ExecutionStatus.TERMINAL).context(new HashMap<>()).outputs(outputs).build();
}

outputs.put("resolvedExpectedArtifacts", Collections.singletonList(expectedArtifact));
outputs.put("artifacts", Collections.singletonList(match));

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs, outputs);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).outputs(outputs).build();
}

@Data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public TaskResult execute(@Nonnull Stage stage) {
+ stageData.location);
}

return new TaskResult(ExecutionStatus.SUCCEEDED, outputs, outputs);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(outputs).outputs(outputs).build();
}

public static class StageData {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public TaskResult execute(@NotNull Stage stage) {
result.getOrDefault("tables", emptyList())
);

return new TaskResult(ExecutionStatus.SUCCEEDED, result);
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(result).build();
} catch (RetrofitError e) {
Map<String, Object> output = new HashMap<>();
List<String> errors = new ArrayList<>();
Expand All @@ -60,7 +60,7 @@ public TaskResult execute(@NotNull Stage stage) {
errors.add(e.getMessage());
}
output.put("errors", errors);
return new TaskResult(ExecutionStatus.TERMINAL, output);
return TaskResult.builder(ExecutionStatus.TERMINAL).context(output).build();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,12 @@ abstract class AbstractClusterWideClouddriverTask extends AbstractCloudProviderA
}

def taskId = katoService.requestOperations(clusterSelection.cloudProvider, katoOps).toBlocking().first()
new TaskResult(ExecutionStatus.SUCCEEDED, [
TaskResult.builder(ExecutionStatus.SUCCEEDED).context([
"notification.type" : getNotificationType(),
"deploy.account.name" : clusterSelection.credentials,
"kato.last.task.id" : taskId,
"deploy.server.groups": locationGroups
])
]).build()
}

private static boolean shouldSkipTrafficGuardCheck(List<Map<String, Map>> katoOps) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ abstract class AbstractWaitForClusterWideClouddriverTask extends AbstractCloudPr

if (stillRemaining) {
log.info "Pipeline ${stage.execution?.id} still has ${stillRemaining.collect { it.region + "->" + it.name }}"
return new TaskResult(ExecutionStatus.RUNNING, [remainingDeployServerGroups: stillRemaining])
return TaskResult.builder(ExecutionStatus.RUNNING).context([remainingDeployServerGroups: stillRemaining]).build()
}

log.info "Pipeline ${stage.execution?.id} no server groups remain"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public TaskResult execute(@Nonnull Stage stage) {
stageData.serverGroup,
e
);
return new TaskResult(ExecutionStatus.RUNNING);
return TaskResult.RUNNING;
}
}

Expand All @@ -147,7 +147,7 @@ public TaskResult execute(@Nonnull Stage stage) {
stageData.cloudProvider,
e
);
return new TaskResult(ExecutionStatus.RUNNING);
return TaskResult.RUNNING;
}

List<ServerGroup> serverGroups = objectMapper.convertValue(
Expand Down Expand Up @@ -228,14 +228,10 @@ public TaskResult execute(@Nonnull Stage stage) {
}
}

return new TaskResult(
ExecutionStatus.SUCCEEDED,
Collections.singletonMap("imagesToRestore", imagesToRestore),
ImmutableMap.<String, Object>builder()
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context(Collections.singletonMap("imagesToRestore", imagesToRestore)).outputs(ImmutableMap.<String, Object>builder()
.put("rollbackTypes", rollbackTypes)
.put("rollbackContexts", rollbackContexts)
.build()
);
.build()).build();
}

private Map<String, Object> fetchCluster(String application,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
if (!locationsWithMissingImageIds.isEmpty()) {
// signifies that at least one summary was missing image details, let's retry until we see image details
log.warn("One or more locations are missing image details (locations: ${locationsWithMissingImageIds*.value}, cluster: ${config.cluster}, account: ${account}, executionId: ${stage.execution.id})")
return new TaskResult(ExecutionStatus.RUNNING)
return TaskResult.ofStatus(ExecutionStatus.RUNNING)
}

if (missingLocations) {
Expand Down Expand Up @@ -292,12 +292,12 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
return artifact
}.flatten()

return new TaskResult(ExecutionStatus.SUCCEEDED, [
return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([
amiDetails: deploymentDetails,
artifacts: artifacts
], [
]).outputs([
deploymentDetails: deploymentDetails
])
]).build()
}

private void resolveFromBaseImageName(
Expand Down
Loading

0 comments on commit 4fc492e

Please sign in to comment.