From 234bad05dc5976e68acb7d1356c386244eeeaa18 Mon Sep 17 00:00:00 2001 From: Eythor Magnusson Date: Mon, 16 Oct 2023 09:45:34 +0200 Subject: [PATCH] improvement(api): send action type to cloud Before this improvement we weren't sending the action type to Cloud due to some legacy issues which have since been resolved. The action type is used in Cloud in e.g. the graph UI and on command results. Note that Cloud has already been updated to handle the action type and those changes rolled out to all users, so this is only required Core side work. --- core/src/actions/types.ts | 2 +- core/src/events/util.ts | 2 +- core/src/graph/actions.ts | 46 ++++++--- core/src/graph/config-graph.ts | 19 ++-- core/src/util/testing.ts | 14 +++ .../kubernetes-type/module-simple/garden.yml | 4 +- .../kubernetes/kubernetes-type/handlers.ts | 2 +- .../src/actions/action-configs-to-graph.ts | 5 + core/test/unit/src/config-graph.ts | 98 ++++++++++++------- core/test/unit/src/garden.ts | 3 + .../unit/src/plugins/container/container.ts | 6 +- core/test/unit/src/tasks/build.ts | 8 +- core/test/unit/src/tasks/deploy.ts | 8 +- core/test/unit/src/tasks/run.ts | 8 +- 14 files changed, 152 insertions(+), 73 deletions(-) diff --git a/core/src/actions/types.ts b/core/src/actions/types.ts index 7520fceb54..3ea3f3e510 100644 --- a/core/src/actions/types.ts +++ b/core/src/actions/types.ts @@ -134,7 +134,7 @@ export interface ActionDependencyAttributes { needsExecutedOutputs: boolean // Set to true if action cannot be resolved without the dependency executed } -export type ActionDependency = ActionReference & ActionDependencyAttributes +export type ActionDependency = ActionReference & ActionDependencyAttributes & { type: string } export interface ActionModes { sync?: boolean diff --git a/core/src/events/util.ts b/core/src/events/util.ts index d8df73cca2..b0bf238996 100644 --- a/core/src/events/util.ts +++ b/core/src/events/util.ts @@ -75,8 +75,8 @@ export function makeActionStatusPayloadBase({ actionName: action.name, actionVersion: action.versionString(), // NOTE: The type/kind needs to be lower case in the event payload - actionType: action.kind.toLowerCase(), actionKind: action.kind.toLowerCase(), + actionType: action.type, actionUid: action.uid, moduleName: action.moduleName(), sessionId, diff --git a/core/src/graph/actions.ts b/core/src/graph/actions.ts index ed2b489c8b..9ea2ace746 100644 --- a/core/src/graph/actions.ts +++ b/core/src/graph/actions.ts @@ -40,7 +40,7 @@ import { describeSchema, parseActionReference } from "../config/common.js" import type { GroupConfig } from "../config/group.js" import { ActionConfigContext } from "../config/template-contexts/actions.js" import { validateWithPath } from "../config/validation.js" -import { ConfigurationError, PluginError, InternalError, ValidationError, GardenError } from "../exceptions.js" +import { ConfigurationError, PluginError, InternalError, GardenError } from "../exceptions.js" import { overrideVariables, type Garden } from "../garden.js" import type { Log } from "../logger/log-entry.js" import type { ActionTypeDefinition } from "../plugin/action-types.js" @@ -916,17 +916,27 @@ function dependenciesFromActionConfig({ } const deps: ActionDependency[] = config.dependencies.map((d) => { - try { - const { kind, name } = parseActionReference(d) - return { kind, name, explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false } - } catch (error) { - throw new ValidationError({ - message: `Invalid dependency specified: ${error}`, + const { kind, name } = parseActionReference(d) + const depKey = actionReferenceToString(d) + const depConfig = configsByKey[depKey] + + if (!depConfig) { + throw new ConfigurationError({ + message: `${description} references depdendency ${depKey}, but no such action could be found`, }) } + + return { + kind, + name, + type: depConfig.type, + explicit: true, + needsExecutedOutputs: false, + needsStaticOutputs: false, + } }) - function addDep(ref: ActionReference, attributes: ActionDependencyAttributes) { + function addDep(ref: ActionReference & { type: string }, attributes: ActionDependencyAttributes) { addActionDependency({ ...ref, ...attributes }, deps) } @@ -943,7 +953,12 @@ function dependenciesFromActionConfig({ }) } - addDep(ref, { explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false }) + const refWithType = { + ...ref, + type: config.type, + } + + addDep(refWithType, { explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false }) } } else if (config.build) { // -> build field on runtime actions @@ -956,7 +971,11 @@ function dependenciesFromActionConfig({ }) } - addDep(ref, { explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false }) + const refWithType = { + ...ref, + type: config.type, + } + addDep(refWithType, { explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false }) } // Action template references in spec/variables @@ -1003,7 +1022,12 @@ function dependenciesFromActionConfig({ needsExecuted = true } - addDep(ref, { explicit: false, needsExecutedOutputs: needsExecuted, needsStaticOutputs: !needsExecuted }) + const refWithType = { + ...ref, + type: refActionType, + } + + addDep(refWithType, { explicit: false, needsExecutedOutputs: needsExecuted, needsStaticOutputs: !needsExecuted }) } return deps diff --git a/core/src/graph/config-graph.ts b/core/src/graph/config-graph.ts index 33348d6ec5..5e2f956015 100644 --- a/core/src/graph/config-graph.ts +++ b/core/src/graph/config-graph.ts @@ -38,6 +38,7 @@ export type RenderedEdge = { dependant: RenderedNode; dependency: RenderedNode } export interface RenderedNode { kind: ActionKind name: string + type: string moduleName?: string key: string disabled: boolean @@ -481,19 +482,20 @@ export abstract class BaseConfigGraph< protected addActionInternal(action: Action) { this.actions[action.kind][action.name] = >action - const node = this.getNode(action.kind, action.name, action.isDisabled()) + const node = this.getNode(action.kind, action.type, action.name, action.isDisabled()) for (const dep of action.getDependencyReferences()) { this.addRelation({ dependant: node, dependencyKind: dep.kind, + dependencyType: dep.type, dependencyName: dep.name, }) } } // Idempotent. - protected getNode(kind: ActionKind, name: string, disabled: boolean) { + protected getNode(kind: ActionKind, type: string, name: string, disabled: boolean) { const key = nodeKey(kind, name) const existingNode = this.dependencyGraph[key] if (existingNode) { @@ -502,7 +504,7 @@ export abstract class BaseConfigGraph< } return existingNode } else { - const newNode = new ConfigGraphNode(kind, name, disabled) + const newNode = new ConfigGraphNode(kind, type, name, disabled) this.dependencyGraph[key] = newNode return newNode } @@ -512,13 +514,15 @@ export abstract class BaseConfigGraph< protected addRelation({ dependant, dependencyKind, + dependencyType, dependencyName, }: { dependant: ConfigGraphNode dependencyKind: ActionKind + dependencyType: string dependencyName: string }) { - const dependency = this.getNode(dependencyKind, dependencyName, false) + const dependency = this.getNode(dependencyKind, dependencyType, dependencyName, false) dependant.addDependency(dependency) dependency.addDependant(dependant) } @@ -543,11 +547,12 @@ export class MutableConfigGraph extends ConfigGraph { const dependant = this.getActionByRef(by) const dependency = this.getActionByRef(on) - dependant.addDependency({ kind: dependency.kind, name: dependency.name, ...attributes }) + dependant.addDependency({ kind: dependency.kind, type: dependency.type, name: dependency.name, ...attributes }) this.addRelation({ - dependant: this.getNode(dependant.kind, dependant.name, dependant.isDisabled()), + dependant: this.getNode(dependant.kind, dependant.type, dependant.name, dependant.isDisabled()), dependencyKind: dependency.kind, + dependencyType: dependency.type, dependencyName: dependency.name, }) } @@ -568,6 +573,7 @@ export class ConfigGraphNode { constructor( public kind: ActionKind, + public type: string, public name: string, public disabled: boolean ) { @@ -581,6 +587,7 @@ export class ConfigGraphNode { kind: this.kind, key: this.name, disabled: this.disabled, + type: this.type, } } diff --git a/core/src/util/testing.ts b/core/src/util/testing.ts index 63975482ed..c1724339ce 100644 --- a/core/src/util/testing.ts +++ b/core/src/util/testing.ts @@ -300,11 +300,25 @@ export class TestGarden extends Garden { this.addActionConfig(config) } + /** + * Replace all module configs with the one provided. + */ setModuleConfigs(moduleConfigs: PartialModuleConfig[]) { this.state.configsScanned = true this.moduleConfigs = keyBy(moduleConfigs.map(moduleConfigWithDefaults), "name") } + /** + * Merge the provided module configs with the existing ones. + */ + mergeModuleConfigs(moduleConfigs: PartialModuleConfig[]) { + this.state.configsScanned = true + this.moduleConfigs = { + ...this.moduleConfigs, + ...keyBy(moduleConfigs.map(moduleConfigWithDefaults), "name"), + } + } + setActionConfigs(actionConfigs: PartialActionConfig[]) { this.actionConfigs = { Build: {}, diff --git a/core/test/data/test-projects/kubernetes-type/module-simple/garden.yml b/core/test/data/test-projects/kubernetes-type/module-simple/garden.yml index 8b491c45a7..4b8feab03b 100644 --- a/core/test/data/test-projects/kubernetes-type/module-simple/garden.yml +++ b/core/test/data/test-projects/kubernetes-type/module-simple/garden.yml @@ -43,7 +43,6 @@ tasks: command: [sh, -c, "echo ok"] --- - kind: Run name: echo-run-exec type: kubernetes-exec @@ -56,7 +55,6 @@ spec: command: [echo, ok] --- - kind: Test name: echo-test-exec type: kubernetes-exec @@ -66,4 +64,4 @@ spec: resource: kind: Deployment name: busybox-deployment - command: [echo, ok] \ No newline at end of file + command: [echo, ok] diff --git a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts index 6ebfbaf4c2..c2d01ddff5 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -115,7 +115,7 @@ describe("kubernetes-type handlers", () => { } async function deployInNamespace({ nsName, deployName }: { nsName: string; deployName: string }) { - garden.setModuleConfigs([withNamespace(nsModuleConfig, nsName)]) + garden.mergeModuleConfigs([withNamespace(nsModuleConfig, nsName)]) const graph = await garden.getConfigGraph({ log, emit: false }) const action = graph.getDeploy(deployName) const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) diff --git a/core/test/unit/src/actions/action-configs-to-graph.ts b/core/test/unit/src/actions/action-configs-to-graph.ts index f0b995c939..3cecfa7b59 100644 --- a/core/test/unit/src/actions/action-configs-to-graph.ts +++ b/core/test/unit/src/actions/action-configs-to-graph.ts @@ -237,6 +237,7 @@ describe("actionConfigsToGraph", () => { explicit: true, kind: "Build", name: "foo", + type: "test", needsExecutedOutputs: false, needsStaticOutputs: false, }, @@ -283,6 +284,7 @@ describe("actionConfigsToGraph", () => { { explicit: true, kind: "Build", + type: "test", name: "foo", needsExecutedOutputs: false, needsStaticOutputs: false, @@ -331,6 +333,7 @@ describe("actionConfigsToGraph", () => { { explicit: false, kind: "Build", + type: "test", name: "foo", fullRef: ["actions", "build", "foo", "version"], needsExecutedOutputs: false, @@ -380,6 +383,7 @@ describe("actionConfigsToGraph", () => { { explicit: false, kind: "Build", + type: "test", name: "foo", fullRef: ["actions", "build", "foo", "outputs", "bar"], needsExecutedOutputs: true, @@ -429,6 +433,7 @@ describe("actionConfigsToGraph", () => { { explicit: false, kind: "Build", + type: "container", name: "foo", fullRef: ["actions", "build", "foo", "outputs", "deploymentImageName"], needsExecutedOutputs: false, diff --git a/core/test/unit/src/config-graph.ts b/core/test/unit/src/config-graph.ts index def91ff5a0..52c27cee0e 100644 --- a/core/test/unit/src/config-graph.ts +++ b/core/test/unit/src/config-graph.ts @@ -29,6 +29,7 @@ import { DEFAULT_BUILD_TIMEOUT_SEC, GARDEN_CORE_ROOT, GardenApiVersion } from ". import type tmp from "tmp-promise" import type { ActionKind, BaseActionConfig } from "../../../src/actions/types.js" import { GraphError, ParameterError } from "../../../src/exceptions.js" +import { sortBy } from "lodash-es" const makeAction = ({ basePath, @@ -1476,90 +1477,112 @@ describe("ConfigGraph (module-based configs)", () => { describe("render", () => { it("should render config graph nodes with test names", () => { const rendered = graphA.render() - expect(rendered.nodes).to.include.deep.members([ + const sortedNodes = sortBy(rendered.nodes, "key") + expect(sortedNodes).to.include.deep.members([ { kind: "Build", name: "module-a", key: "module-a", disabled: false, + type: "test", + }, + { + kind: "Test", + name: "module-a-integration", + key: "module-a-integration", + disabled: false, + type: "test", + }, + { + kind: "Test", + name: "module-a-unit", + key: "module-a-unit", + disabled: false, + type: "test", }, { kind: "Build", name: "module-b", key: "module-b", disabled: false, + type: "test", }, { - kind: "Build", - name: "module-c", - key: "module-c", + kind: "Test", + name: "module-b-unit", + key: "module-b-unit", disabled: false, + type: "test", }, { - kind: "Test", - name: "module-c-unit", - key: "module-c-unit", + kind: "Build", + name: "module-c", + key: "module-c", disabled: false, + type: "test", }, { kind: "Test", name: "module-c-integ", key: "module-c-integ", disabled: false, + type: "test", }, { - kind: "Run", - name: "task-c", - key: "task-c", + kind: "Test", + name: "module-c-unit", + key: "module-c-unit", disabled: false, + type: "test", }, { kind: "Deploy", - name: "service-c", - key: "service-c", + name: "service-a", + key: "service-a", disabled: false, + type: "test", }, { - kind: "Test", - name: "module-a-unit", - key: "module-a-unit", + kind: "Deploy", + name: "service-b", + key: "service-b", disabled: false, + type: "test", }, { - kind: "Test", - name: "module-a-integration", - key: "module-a-integration", + kind: "Deploy", + name: "service-c", + key: "service-c", disabled: false, + type: "test", }, { kind: "Run", name: "task-a", key: "task-a", disabled: false, + type: "test", }, { - kind: "Test", - name: "module-b-unit", - key: "module-b-unit", + name: "task-a2", + kind: "Run", + key: "task-a2", disabled: false, + type: "test", }, { kind: "Run", name: "task-b", key: "task-b", disabled: false, + type: "test", }, { - kind: "Deploy", - name: "service-a", - key: "service-a", - disabled: false, - }, - { - kind: "Deploy", - name: "service-b", - key: "service-b", + kind: "Run", + name: "task-c", + key: "task-c", disabled: false, + type: "test", }, ]) }) @@ -1569,57 +1592,62 @@ describe("ConfigGraph (module-based configs)", () => { describe("ConfigGraphNode", () => { describe("render", () => { it("should render a build node", () => { - const node = new ConfigGraphNode("Build", "module-a", false) + const node = new ConfigGraphNode("Build", "container", "module-a", false) const res = node.render() expect(res).to.eql({ kind: "Build", name: "module-a", key: "module-a", disabled: false, + type: "container", }) }) it("should render a deploy node", () => { - const node = new ConfigGraphNode("Deploy", "service-a", false) + const node = new ConfigGraphNode("Deploy", "container", "service-a", false) const res = node.render() expect(res).to.eql({ kind: "Deploy", name: "service-a", key: "service-a", disabled: false, + type: "container", }) }) it("should render a run node", () => { - const node = new ConfigGraphNode("Run", "task-a", false) + const node = new ConfigGraphNode("Run", "container", "task-a", false) const res = node.render() expect(res).to.eql({ kind: "Run", name: "task-a", key: "task-a", disabled: false, + type: "container", }) }) it("should render a test node", () => { - const node = new ConfigGraphNode("Test", "module-a.test-a", false) + const node = new ConfigGraphNode("Test", "container", "module-a.test-a", false) const res = node.render() expect(res).to.eql({ kind: "Test", name: "module-a.test-a", key: "module-a.test-a", disabled: false, + type: "container", }) }) it("should indicate if the node is disabled", () => { - const node = new ConfigGraphNode("Test", "module-a.test-a", true) + const node = new ConfigGraphNode("Test", "container", "module-a.test-a", true) const res = node.render() expect(res).to.eql({ kind: "Test", name: "module-a.test-a", key: "module-a.test-a", disabled: true, + type: "container", }) }) }) diff --git a/core/test/unit/src/garden.ts b/core/test/unit/src/garden.ts index 6296a78c45..831411f4c6 100644 --- a/core/test/unit/src/garden.ts +++ b/core/test/unit/src/garden.ts @@ -4895,6 +4895,7 @@ describe("Garden", () => { { explicit: true, kind: "Build", + type: "foo", name: "foo", needsExecutedOutputs: false, needsStaticOutputs: false, @@ -4957,6 +4958,7 @@ describe("Garden", () => { { explicit: true, kind: "Build", + type: "test", name: "foo", needsExecutedOutputs: false, needsStaticOutputs: false, @@ -5086,6 +5088,7 @@ describe("Garden", () => { { explicit: true, kind: "Build", + type: "foo", name: "foo", needsExecutedOutputs: false, needsStaticOutputs: false, diff --git a/core/test/unit/src/plugins/container/container.ts b/core/test/unit/src/plugins/container/container.ts index 339e98758c..9045941361 100644 --- a/core/test/unit/src/plugins/container/container.ts +++ b/core/test/unit/src/plugins/container/container.ts @@ -118,15 +118,15 @@ describe("plugins.container", () => { basePath: "module-a", }, }, - convertBuildDependency: () => ({ kind: "Build", name: "buildDep" }), - convertRuntimeDependencies: () => [{ kind: "Deploy", name: "runtimeDep" }], + convertBuildDependency: () => ({ kind: "Build", type: "container", name: "buildDep" }), + convertRuntimeDependencies: () => [{ kind: "Deploy", type: "container", name: "runtimeDep" }], convertTestName: () => "testName", ctx, dummyBuild: undefined, log, module, prepareRuntimeDependencies: prepareRuntimeDependencies - ? () => [{ kind: "Deploy", name: "preopRuntimeDep" }] + ? () => [{ kind: "Deploy", type: "container", name: "preopRuntimeDep" }] : () => [], services, tasks, diff --git a/core/test/unit/src/tasks/build.ts b/core/test/unit/src/tasks/build.ts index 3feb1b40f5..bd37628474 100644 --- a/core/test/unit/src/tasks/build.ts +++ b/core/test/unit/src/tasks/build.ts @@ -119,7 +119,7 @@ describe("BuildTask", () => { payload: { actionName: "test-build", actionVersion, - actionType: "build", + actionType: "test", actionKind: "build", actionUid, moduleName: null, @@ -137,7 +137,7 @@ describe("BuildTask", () => { payload: { actionName: "test-build", actionVersion, - actionType: "build", + actionType: "test", actionKind: "build", actionUid, moduleName: null, @@ -156,7 +156,7 @@ describe("BuildTask", () => { payload: { actionName: "test-build", actionVersion, - actionType: "build", + actionType: "test", actionKind: "build", actionUid, moduleName: null, @@ -174,7 +174,7 @@ describe("BuildTask", () => { payload: { actionName: "test-build", actionVersion, - actionType: "build", + actionType: "test", actionKind: "build", actionUid, moduleName: null, diff --git a/core/test/unit/src/tasks/deploy.ts b/core/test/unit/src/tasks/deploy.ts index c38c4d03a1..991c61b6e9 100644 --- a/core/test/unit/src/tasks/deploy.ts +++ b/core/test/unit/src/tasks/deploy.ts @@ -216,7 +216,7 @@ describe("DeployTask", () => { payload: { actionName: "test-deploy", actionVersion, - actionType: "deploy", + actionType: "test", actionKind: "deploy", actionUid, moduleName: null, @@ -234,7 +234,7 @@ describe("DeployTask", () => { payload: { actionName: "test-deploy", actionVersion, - actionType: "deploy", + actionType: "test", actionKind: "deploy", actionUid, moduleName: null, @@ -258,7 +258,7 @@ describe("DeployTask", () => { payload: { actionName: "test-deploy", actionVersion, - actionType: "deploy", + actionType: "test", actionKind: "deploy", actionUid, moduleName: null, @@ -276,7 +276,7 @@ describe("DeployTask", () => { payload: { actionName: "test-deploy", actionVersion, - actionType: "deploy", + actionType: "test", actionKind: "deploy", actionUid, moduleName: null, diff --git a/core/test/unit/src/tasks/run.ts b/core/test/unit/src/tasks/run.ts index a4d7f0f6ee..0e16b7774b 100644 --- a/core/test/unit/src/tasks/run.ts +++ b/core/test/unit/src/tasks/run.ts @@ -163,7 +163,7 @@ describe("RunTask", () => { payload: { actionName: "test", actionVersion, - actionType: "run", + actionType: "test", actionKind: "run", actionUid, moduleName: null, @@ -181,7 +181,7 @@ describe("RunTask", () => { payload: { actionName: "test", actionVersion, - actionType: "run", + actionType: "test", actionKind: "run", actionUid, moduleName: null, @@ -200,7 +200,7 @@ describe("RunTask", () => { payload: { actionName: "test", actionVersion, - actionType: "run", + actionType: "test", actionKind: "run", actionUid, moduleName: null, @@ -218,7 +218,7 @@ describe("RunTask", () => { payload: { actionName: "test", actionVersion, - actionType: "run", + actionType: "test", actionKind: "run", actionUid, moduleName: null,