From 4a2cd9c25a6b0ad5c9a42b3c26649797ef1a7816 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Mon, 17 Feb 2025 08:50:17 +0100 Subject: [PATCH] fix(dev): fix unresolved templates in cmd results (#6850) * fix(dev): fix warning message during init * fix(dev): fix unresolved templates in cmd results We were including the raw graph results in the results of commands requested over HTTP from the live page. This was always unnecessary (and the data structures involved were excessively large), but only started resulting in errors with our recent templating improvements (previously, we'd just leave the unresolved template strings in there). We can clean this up further in 0.14 when we get rid of `graphResults` from `ProcessCommandResult` (which will require fixing a bunch of test cases which currently use that field). --- core/src/actions/base.ts | 4 +-- core/src/actions/types.ts | 4 +-- core/src/commands/base.ts | 36 +++++++++++++------- core/src/commands/get/get-actions.ts | 4 +-- core/src/commands/serve.ts | 1 + core/src/events/action-status-events.ts | 4 +-- core/src/plugin/base.ts | 5 +-- core/src/plugin/handlers/Build/get-status.ts | 3 +- core/src/server/server.ts | 7 ++-- core/src/util/ink-divider.tsx | 20 +++++------ core/src/util/logging.ts | 7 +++- core/test/unit/src/server/server.ts | 24 +++++++------ 12 files changed, 69 insertions(+), 50 deletions(-) diff --git a/core/src/actions/base.ts b/core/src/actions/base.ts index 0b33aa3d78..4dba9509c1 100644 --- a/core/src/actions/base.ts +++ b/core/src/actions/base.ts @@ -54,7 +54,7 @@ import type { ResolvedAction, ResolvedActionWrapperParams, } from "./types.js" -import { actionKinds, actionStateTypes } from "./types.js" +import { actionKinds, actionStates } from "./types.js" import { baseInternalFieldsSchema, varfileDescription } from "../config/base.js" import type { DeployAction } from "./deploy.js" import type { TestAction } from "./test.js" @@ -298,7 +298,7 @@ export const actionStatusSchema = createSchema({ keys: () => ({ state: joi .string() - .allow(...actionStateTypes) + .allow(...actionStates) .only() .required() .description("The state of the action."), diff --git a/core/src/actions/types.ts b/core/src/actions/types.ts index 63c4cf905d..746a0e3e06 100644 --- a/core/src/actions/types.ts +++ b/core/src/actions/types.ts @@ -112,8 +112,8 @@ export interface ActionConfigTypes { * * See https://melvingeorge.me/blog/convert-array-into-string-literal-union-type-typescript */ -export const actionStateTypes = ["ready", "not-ready", "processing", "failed", "unknown"] as const -export type ActionState = (typeof actionStateTypes)[number] +export const actionStates = ["ready", "not-ready", "processing", "failed", "unknown"] as const +export type ActionState = (typeof actionStates)[number] export interface ActionStatus< T extends BaseAction = BaseAction, diff --git a/core/src/commands/base.ts b/core/src/commands/base.ts index 9faa9d8ded..1733540b0a 100644 --- a/core/src/commands/base.ts +++ b/core/src/commands/base.ts @@ -35,7 +35,7 @@ import type { DeployState, ForwardablePort, ServiceIngress } from "../types/serv import { deployStates, forwardablePortSchema, serviceIngressSchema } from "../types/service.js" import type { GraphResultMapWithoutTask, GraphResultWithoutTask, GraphResults } from "../graph/results.js" import { splitFirst } from "../util/string.js" -import type { ActionMode } from "../actions/types.js" +import { actionStates, type ActionMode, type ActionState } from "../actions/types.js" import type { AnalyticsHandler } from "../analytics/analytics.js" import { withSessionContext } from "../util/open-telemetry/context.js" import { wrapActiveSpan } from "../util/open-telemetry/spans.js" @@ -699,7 +699,7 @@ ${renderCommands(commands)} // fixme: These interfaces and schemas are mostly copied from their original locations. This is to ensure that // dynamically sized or nested fields don't accidentally get introduced to command results. We should find a neater -// wat to manage all this. +// way to manage all this. interface BuildResultForExport extends ProcessResultMetadata { buildLog?: string @@ -730,6 +730,7 @@ interface DeployResultForExport extends ProcessResultMetadata { lastMessage?: string lastError?: string outputs?: PrimitiveMap + // TODO-0.14: Rename to deployState state: DeployState } @@ -756,6 +757,7 @@ const deployResultForExportSchema = createSchema({ lastError: joi.string().description("Latest error status message of the service (if any)."), outputs: joiVariables().description("A map of values output from the deployment."), runningReplicas: joi.number().description("How many replicas of the service are currently running."), + // TODO-0.14: Rename to deployState state: joi .string() .valid(...deployStates) @@ -790,8 +792,8 @@ interface TestResultForExport extends ProcessResultMetadata { const testResultForExportSchema = createSchema({ name: "test-result-for-export", - keys: () => ({}), extend: runResultForExportSchema, + keys: () => ({}), }) export type ProcessResultMetadata = { @@ -800,20 +802,21 @@ export type ProcessResultMetadata = { success: boolean error?: string inputVersion: string | null + actionState: ActionState } export interface ProcessCommandResult { aborted: boolean success: boolean - graphResults: GraphResultMapWithoutTask // TODO: Remove this. + graphResults: GraphResultMapWithoutTask // TODO: Remove this in 0.14. build: { [name: string]: BuildResultForExport } builds: { [name: string]: BuildResultForExport } deploy: { [name: string]: DeployResultForExport } - deployments: { [name: string]: DeployResultForExport } // alias for backwards-compatibility + deployments: { [name: string]: DeployResultForExport } // alias for backwards-compatibility (remove in 0.14) test: { [name: string]: TestResultForExport } tests: { [name: string]: TestResultForExport } run: { [name: string]: RunResultForExport } - tasks: { [name: string]: RunResultForExport } // alias for backwards-compatibility + tasks: { [name: string]: RunResultForExport } // alias for backwards-compatibility (remove in 0.14) } export const resultMetadataKeys = () => ({ @@ -831,6 +834,7 @@ export const resultMetadataKeys = () => ({ .description( "Alias for `inputVersion`. The version of the task's inputs, before any resolution or execution happens. For action tasks, this will generally be the unresolved version." ), + actionState: joi.string().valid(...actionStates), outputs: joiVariables().description("A map of values output from the action's execution."), }) @@ -842,11 +846,11 @@ export const processCommandResultSchema = createSchema({ // Hide this field from the docs, since we're planning to remove it. graphResults: joi.any().meta({ internal: true }), build: joiIdentifierMap(buildResultForExportSchema().keys(resultMetadataKeys())) - .description("A map of all executed Builds (or Builds scheduled/attempted) and information about the them.") + .description("A map of all executed Builds (or Builds scheduled/attempted) and information about them.") .meta({ keyPlaceholder: "" }), builds: joiIdentifierMap(buildResultForExportSchema().keys(resultMetadataKeys())) .description( - "Alias for `build`. A map of all executed Builds (or Builds scheduled/attempted) and information about the them." + "Alias for `build`. A map of all executed Builds (or Builds scheduled/attempted) and information about them." ) .meta({ keyPlaceholder: "", deprecated: true }), deploy: joiIdentifierMap(deployResultForExportSchema().keys(resultMetadataKeys())) @@ -929,8 +933,8 @@ function prepareBuildResult(graphResult: GraphResultWithoutTask): BuildResultFor if (buildResult) { return { ...common, - buildLog: buildResult && buildResult.buildLog, - fresh: buildResult && buildResult.fresh, + buildLog: buildResult.buildLog, + fresh: buildResult.fresh, } } else { return common @@ -976,7 +980,9 @@ function prepareDeployResult(graphResult: GraphResultWithoutTask): DeployResultF } function prepareTestResult(graphResult: GraphResultWithoutTask): TestResultForExport & ProcessResultMetadata { - const common = commonResultFields(graphResult) + const common = { + ...commonResultFields(graphResult), + } const detail = graphResult.result?.detail if (detail) { return { @@ -992,7 +998,9 @@ function prepareTestResult(graphResult: GraphResultWithoutTask): TestResultForEx } function prepareRunResult(graphResult: GraphResultWithoutTask): RunResultForExport & ProcessResultMetadata { - const common = commonResultFields(graphResult) + const common = { + ...commonResultFields(graphResult), + } const detail = graphResult.result?.detail if (detail) { return { @@ -1016,6 +1024,7 @@ function commonResultFields(graphResult: GraphResultWithoutTask) { inputVersion: graphResult.inputVersion, // Here for backwards-compatibility version: graphResult.inputVersion, + actionState: graphResult.result.state as ActionState, } } @@ -1051,7 +1060,8 @@ export async function handleProcessResults( const result: ProcessCommandResult = { aborted: false, success, - graphResults: graphResultsForExport, // TODO: Remove this. + // TODO-0.14: Remove graphResults from this type (will also require refactoring test cases that read from this field) + graphResults: graphResultsForExport, build: buildResults, builds: buildResults, // alias for `build` deploy: deployResults, diff --git a/core/src/commands/get/get-actions.ts b/core/src/commands/get/get-actions.ts index 404cfae364..07018972f2 100644 --- a/core/src/commands/get/get-actions.ts +++ b/core/src/commands/get/get-actions.ts @@ -8,7 +8,7 @@ import { getActionState, getRelativeActionConfigPath } from "../../actions/helpers.js" import type { ActionKind, ActionState, ResolvedAction } from "../../actions/types.js" -import { actionKinds, actionStateTypes } from "../../actions/types.js" +import { actionKinds, actionStates } from "../../actions/types.js" import { BooleanParameter, ChoicesParameter, StringsParameter } from "../../cli/params.js" import { createSchema, joi, joiArray } from "../../config/common.js" import { printHeader } from "../../logger/util.js" @@ -50,7 +50,7 @@ export const getActionsCmdOutputSchema = createSchema({ type: joi.string().required().description(`Action Type (e.g. 'container').`), state: joi .string() - .allow(...actionStateTypes) + .allow(...actionStates) .only() .description("The state of the action."), path: joi.string().description("The relative path of the action config file."), diff --git a/core/src/commands/serve.ts b/core/src/commands/serve.ts index 399ed95217..9847e0d1d6 100644 --- a/core/src/commands/serve.ts +++ b/core/src/commands/serve.ts @@ -186,6 +186,7 @@ export class ServeCommand< const distroName = getCloudDistributionName(defaultGarden.cloudDomain) const livePageUrl = cloudApi.getLivePageUrl({ shortId: session.shortId }).toString() const msg = dedent`\n${printEmoji("🌸", log)}Connected to ${distroName} ${printEmoji("🌸", log)} + Follow the link below to stream logs, run commands, and more from the Garden dashboard ${printEmoji( "👇", log diff --git a/core/src/events/action-status-events.ts b/core/src/events/action-status-events.ts index fb1e958867..268de3be4d 100644 --- a/core/src/events/action-status-events.ts +++ b/core/src/events/action-status-events.ts @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { actionStateTypes } from "../actions/types.js" +import { actionStates } from "../actions/types.js" import type { BuildState } from "../plugin/handlers/Build/get-status.js" import type { ActionRuntime, RunState } from "../plugin/plugin.js" import type { DeployState } from "../types/service.js" @@ -18,7 +18,7 @@ export type ActionStatusDetailedState = DeployState | BuildState | RunState * These are the states emitted in status events. Here, we include additional states to help distinguish status event * emitted around status/cache checks VS statuses emitted around the execution after a failed status check. */ -const actionStateTypesForEvent = [...actionStateTypes, "getting-status", "cached"] as const +const actionStateTypesForEvent = [...actionStates, "getting-status", "cached"] as const /** * This type represents the lifecycle of an individual action execution as emitted to Cloud. Note that the * internal semantics are slightly different (e.g. Garden uses "ready" instead of "cached" internally). diff --git a/core/src/plugin/base.ts b/core/src/plugin/base.ts index 14131cca64..98764511d6 100644 --- a/core/src/plugin/base.ts +++ b/core/src/plugin/base.ts @@ -182,14 +182,15 @@ export const artifactsPathSchema = memoize(() => joi.string().required().description("A directory path where the handler should write any exported artifacts to.") ) -export type RunState = "outdated" | "unknown" | "running" | "succeeded" | "failed" | "not-implemented" +export const runStates = ["outdated", "unknown", "running", "succeeded", "failed", "not-implemented"] as const +export type RunState = (typeof runStates)[number] export interface RunStatusForEventPayload { state: RunState } export const outputSchemaDocs = dedent` - The schema must be a single level object, with string keys. Each value must be a primitive (null, boolean, number or string). + The schema must be a single level object, with string keys. Each vaue must be a primitive (null, boolean, number or string). If no schema is provided, an error may be thrown if a plugin handler attempts to return an output key. diff --git a/core/src/plugin/handlers/Build/get-status.ts b/core/src/plugin/handlers/Build/get-status.ts index ac893bc006..31ad59109e 100644 --- a/core/src/plugin/handlers/Build/get-status.ts +++ b/core/src/plugin/handlers/Build/get-status.ts @@ -21,7 +21,8 @@ import type { ActionStatus, ActionStatusMap, Resolved } from "../../../actions/t * - `built`: The build was completed successfully. * - `failed`: An error occurred while fetching or building. */ -export type BuildState = "fetching" | "fetched" | "outdated" | "building" | "built" | "failed" | "unknown" +export const buildStates = ["fetching", "fetched", "outdated", "building", "built", "failed", "unknown"] as const +export type BuildState = (typeof buildStates)[number] export interface BuildStatusForEventPayload { state: BuildState diff --git a/core/src/server/server.ts b/core/src/server/server.ts index 743576c5a7..1090bb2345 100644 --- a/core/src/server/server.ts +++ b/core/src/server/server.ts @@ -233,7 +233,7 @@ export class GardenServer extends EventEmitter { } } while (!serverStarted) } - this.log.info(`Garden server has successfully started at port ${styles.highlight(this.port.toString())}\n`) + this.log.info(`Garden server has successfully started at port ${styles.highlight(this.port.toString())}`) const processRecord = await this.globalConfigStore.get("activeProcesses", String(process.pid)) @@ -380,7 +380,6 @@ export class GardenServer extends EventEmitter { parentSessionId: this.sessionId, }) this.debugLog.debug(`Command '${command.name}' completed successfully`) - ctx.response.body = sanitizeValue(result) } catch (error) { // Return 200 with errors attached, since commands can legitimately fail (e.g. tests erroring etc.) @@ -706,7 +705,9 @@ export class GardenServer extends EventEmitter { }) // Here we handle the actual command result. .then((commandResult) => { - const { result, errors } = commandResult + const errors = commandResult.errors + // TODO-DODDI-0.14: Remove this line once we've removed graphResults from ProcessCommandResult. + const result = omit(commandResult.result, "graphResults") send( "commandResult", sanitizeValue({ diff --git a/core/src/util/ink-divider.tsx b/core/src/util/ink-divider.tsx index a3a157a2cb..3df5acfb5d 100644 --- a/core/src/util/ink-divider.tsx +++ b/core/src/util/ink-divider.tsx @@ -44,7 +44,15 @@ const getNumberOfCharsPerWidth = (char, width) => width / stringWidth(char) const PAD = " " // Divider -const Divider = ({ title, width, padding, titlePadding, titleColor, dividerChar, dividerColor }) => { +const Divider = ({ + title, + width = 50, + padding = 1, + titlePadding = 1, + titleColor = "white", + dividerChar = "─", + dividerColor = "grey", +}) => { const titleString = title ? `${PAD.repeat(titlePadding) + title + PAD.repeat(titlePadding)}` : "" const titleWidth = stringWidth(titleString) @@ -77,14 +85,4 @@ Divider.propTypes = { dividerColor: PropTypes.string, } -Divider.defaultProps = { - title: null, - width: 50, - padding: 1, - titlePadding: 1, - titleColor: "white", - dividerChar: "─", - dividerColor: "grey", -} - export default Divider diff --git a/core/src/util/logging.ts b/core/src/util/logging.ts index 1e8387e7fc..c85183c3a5 100644 --- a/core/src/util/logging.ts +++ b/core/src/util/logging.ts @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { isArray, isPlainObject, isString, mapValues } from "lodash-es" +import { isArray, isPlainObject, isString, mapValues, omit } from "lodash-es" import stripAnsi from "strip-ansi" import { isPrimitive } from "../config/common.js" import { deepFilter } from "./objects.js" @@ -24,6 +24,11 @@ export function sanitizeValue(value: any, _parents?: WeakSet): any { }) } + // TODO-DODDI-0.14: Remove this line once we've removed graphResults from ProcessCommandResult. + if (isPlainObject(value) && "graphResults" in value) { + value = omit(value, "graphResults") + } + if (!_parents) { _parents = new WeakSet() } else if (_parents.has(value)) { diff --git a/core/test/unit/src/server/server.ts b/core/test/unit/src/server/server.ts index 75e845aea8..bb4a4a68b5 100644 --- a/core/test/unit/src/server/server.ts +++ b/core/test/unit/src/server/server.ts @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { expectError, makeTestGardenA, taskResultOutputs, testPluginReferences } from "../../../helpers.js" +import { expectError, makeTestGardenA, testPluginReferences } from "../../../helpers.js" import type { Server } from "http" import { GardenServer, startServer } from "../../../../src/server/server.js" import type { Garden } from "../../../../src/garden.js" @@ -17,7 +17,7 @@ import { gardenEnv } from "../../../../src/constants.js" import { deepOmitUndefined } from "../../../../src/util/objects.js" import { uuidv4 } from "../../../../src/util/random.js" import { GardenInstanceManager } from "../../../../src/server/instance-manager.js" -import type { CommandParams } from "../../../../src/commands/base.js" +import type { CommandParams, ProcessCommandResult } from "../../../../src/commands/base.js" import { Command } from "../../../../src/commands/base.js" import request from "supertest" import getPort from "get-port" @@ -228,9 +228,9 @@ describe("GardenServer", () => { .expect(200) expect(res.body.errors).to.eq(undefined, `error response: ${res.body.errors?.[0]?.stack}`) - const result = taskResultOutputs(res.body.result) - expect(result["build.module-a"]).to.exist - expect(result["build.module-a"].state).to.equal("ready") + const result = res.body.result as ProcessCommandResult + expect(result.build["module-a"]).to.exist + expect(result.build["module-a"].actionState).to.equal("ready") }) it("creates a Garden instance as needed", async () => { @@ -497,14 +497,15 @@ describe("GardenServer", () => { if (req.type !== "commandResult") { return } - const taskResult = taskResultOutputs(req.result) + const taskResult = req.result const result = { ...req, result: taskResult, } expect(result.requestId).to.equal(id) - expect(result.result["build.module-a"]).to.exist - expect(result.result["build.module-a"].state).to.equal("ready") + const processRes = result.result as ProcessCommandResult + expect(processRes.build["module-a"]).to.exist + expect(processRes.build["module-a"].actionState).to.equal("ready") done() }, skipType: "logEntry", @@ -535,14 +536,15 @@ describe("GardenServer", () => { if (msg.type !== "commandResult") { return } - const taskResult = taskResultOutputs(msg.result) + const taskResult = msg.result const result = { ...msg, result: taskResult, } expect(result.requestId).to.equal(id) - expect(result.result["build.module-a"]).to.exist - expect(result.result["build.module-a"].state).to.equal("ready") + const processRes = result.result as ProcessCommandResult + expect(processRes.build["module-a"]).to.exist + expect(processRes.build["module-a"].actionState).to.equal("ready") done() }, skipType: "logEntry",