Skip to content

Commit

Permalink
fix(core): null reference error when an action with dependants failed
Browse files Browse the repository at this point in the history
Basically we had mistyped the TaskResults object, so I made it
explicit that there may be a null value on a key, which is what happens
when a task is aborted after a dependency fails to execute.
  • Loading branch information
edvald committed Sep 25, 2019
1 parent 8057af7 commit 7c1fb0d
Show file tree
Hide file tree
Showing 11 changed files with 18 additions and 18 deletions.
4 changes: 2 additions & 2 deletions garden-service/src/commands/run/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class RunTaskCommand extends Command<Args, Opts> {

async action(
{ garden, log, headerLog, footerLog, args, opts }: CommandParams<Args, Opts>,
): Promise<CommandResult<TaskResult>> {
): Promise<CommandResult<TaskResult | null>> {
const graph = await garden.getConfigGraph()
const task = await graph.getTask(args.task)

Expand All @@ -62,7 +62,7 @@ export class RunTaskCommand extends Command<Args, Opts> {
const taskTask = await TaskTask.factory({ garden, graph, task, log, force: true, forceBuild: opts["force-build"] })
const result = (await garden.processTasks([taskTask]))[taskTask.getKey()]

if (!result.error) {
if (result && !result.error) {
log.info("")
// TODO: The command will need to be updated to stream logs: see https://github.com/garden-io/garden/issues/630.
// It's ok with the current providers but the shape might change in the future.
Expand Down
4 changes: 2 additions & 2 deletions garden-service/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,14 +499,14 @@ export class Garden {
const failed = Object.values(taskResults).filter(r => r && r.error)

if (failed.length) {
const messages = failed.map(r => `- ${r.name}: ${r.error!.message}`)
const messages = failed.map(r => `- ${r!.name}: ${r!.error!.message}`)
throw new PluginError(
`Failed resolving one or more provider configurations:\n${messages.join("\n")}`,
{ rawConfigs, taskResults, messages },
)
}

const providers: Provider[] = Object.values(taskResults).map(result => result.output)
const providers: Provider[] = Object.values(taskResults).map(result => result!.output)

await Bluebird.map(providers, async (provider) =>
Bluebird.map(provider.moduleConfigs, async (moduleConfig) => {
Expand Down
2 changes: 1 addition & 1 deletion garden-service/src/plugins/kubernetes/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export async function prepareSystem(
}

const serviceStatuses: ServiceStatusMap = (status.detail && status.detail.serviceStatuses) || {}
const serviceStates = Object.values(serviceStatuses).map(s => s.state!)
const serviceStates = Object.values(serviceStatuses).map(s => (s && s.state) || "unknown")
const combinedState = combineStates(serviceStates)

const remoteCluster = provider.name !== "local-kubernetes"
Expand Down
2 changes: 1 addition & 1 deletion garden-service/src/plugins/kubernetes/status/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export async function waitForServiceEndpoints(
while (true) {
const endpoints = await api.core.readNamespacedEndpoints(serviceName, serviceNamespace)

const addresses = flatten(endpoints.subsets!.map(subset => subset.addresses || []))
const addresses = flatten((endpoints.subsets || []).map(subset => subset.addresses || []))
const routedPods = addresses
.filter(a => a.targetRef!.kind === "Pod" && readyPodNames.includes(a.targetRef!.name!))

Expand Down
2 changes: 1 addition & 1 deletion garden-service/src/plugins/kubernetes/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export async function getSystemServiceStatus(
const actions = await sysGarden.getActionHelper()

const serviceStatuses = await actions.getServiceStatuses({ log, serviceNames })
const state = combineStates(values(serviceStatuses).map(s => s.state || "unknown"))
const state = combineStates(values(serviceStatuses).map(s => (s && s.state) || "unknown"))

// Add the Kubernetes dashboard to the Garden dashboard
if (serviceNames.includes("kubernetes-dashboard")) {
Expand Down
2 changes: 1 addition & 1 deletion garden-service/src/task-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export interface TaskResult {
* the result from the last processed is used (hence only one key-value pair here per key).
*/
export interface TaskResults {
[key: string]: TaskResult
[key: string]: TaskResult | null
}

const DEFAULT_CONCURRENCY = 6
Expand Down
12 changes: 6 additions & 6 deletions garden-service/src/tasks/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,19 @@ export abstract class BaseTask {
}

export function getServiceStatuses(dependencyResults: TaskResults): { [name: string]: ServiceStatus } {
const getServiceStatusResults = pickBy(dependencyResults, r => r.type === "get-service-status")
const deployResults = pickBy(dependencyResults, r => r.type === "deploy")
const getServiceStatusResults = pickBy(dependencyResults, r => r && r.type === "get-service-status")
const deployResults = pickBy(dependencyResults, r => r && r.type === "deploy")
// DeployTask results take precedence over GetServiceStatusTask results, because status changes after deployment
const combined = { ...getServiceStatusResults, ...deployResults }
const statuses = mapValues(combined, r => r.output as ServiceStatus)
const statuses = mapValues(combined, r => r!.output as ServiceStatus)
return mapKeys(statuses, (_, key) => splitLast(key, ".")[1])
}

export function getRunTaskResults(dependencyResults: TaskResults): { [name: string]: RunTaskResult } {
const storedResults = pickBy(dependencyResults, r => r.type === "get-task-result")
const runResults = pickBy(dependencyResults, r => r.type === "task")
const storedResults = pickBy(dependencyResults, r => r && r.type === "get-task-result")
const runResults = pickBy(dependencyResults, r => r && r.type === "task")
// TaskTask results take precedence over GetTaskResultTask results
const combined = { ...storedResults, ...runResults }
const results = mapValues(combined, r => r.output as RunTaskResult)
const results = mapValues(combined, r => r!.output as RunTaskResult)
return mapKeys(results, (_, key) => splitLast(key, ".")[1])
}
2 changes: 1 addition & 1 deletion garden-service/src/tasks/resolve-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class ResolveProviderTask extends BaseTask {
}

async process(dependencyResults: TaskResults) {
const resolvedProviders: Provider[] = Object.values(dependencyResults).map(result => result.output)
const resolvedProviders: Provider[] = Object.values(dependencyResults).map(result => result && result.output)

const context = new ProviderConfigContext(this.garden.environmentName, this.garden.projectName, resolvedProviders)

Expand Down
2 changes: 1 addition & 1 deletion garden-service/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ export async function expectError(fn: Function, typeOrCallback?: string | ((err:
}

export function taskResultOutputs(results: TaskResults) {
return mapValues(results, r => r.output)
return mapValues(results, r => r && r.output)
}

export const cleanProject = async (gardenDirPath: string) => {
Expand Down
2 changes: 1 addition & 1 deletion garden-service/test/unit/src/plugins/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ describe("exec plugin", () => {
const results = await _garden.processTasks([taskTask])

// Task A echoes "task-a-output" and Task B echoes the output from Task A
expect(results["task.task-b"].output.outputs.log).to.equal("task-a-output")
expect(results["task.task-b"]!.output.outputs.log).to.equal("task-a-output")
})

describe("getBuildStatus", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,6 @@ describe("Terraform module type", () => {

it("should expose runtime outputs to template contexts", async () => {
const result = await runTestTask()
expect(result["task.test-task"].output.outputs.log).to.equal("input: foo")
expect(result["task.test-task"]!.output.outputs.log).to.equal("input: foo")
})
})

0 comments on commit 7c1fb0d

Please sign in to comment.