fix: add backwards compatibility for old-style run commands (#4195)

* fix: add backwards compatibility for old-style run commands * chore: run command error message improvement from code review Co-authored-by: stefreak <[email protected]> * test: ensure warnings for legacy-style run command invocations * chore: code review suggestion Co-authored-by: Steffen Neubauer <[email protected]> --------- Co-authored-by: stefreak <[email protected]> Co-authored-by: Steffen Neubauer <[email protected]>
garden-io · May 8, 2023 · 8f5218d · 8f5218d
1 parent 0d3fadc
commit 8f5218d
Show file tree

Hide file tree

Showing 5 changed files with 184 additions and 89 deletions.
diff --git a/core/src/commands/run.ts b/core/src/commands/run.ts
@@ -7,13 +7,24 @@
  */
 
 import chalk from "chalk"
-import { Command, CommandParams, handleProcessResults, PrepareParams, processCommandResultSchema } from "./base"
+import {
+  Command,
+  CommandParams,
+  CommandResult,
+  handleProcessResults,
+  PrepareParams,
+  ProcessCommandResult,
+  processCommandResultSchema,
+} from "./base"
 import { RunTask } from "../tasks/run"
 import { printHeader } from "../logger/util"
 import { ParameterError } from "../exceptions"
 import { dedent, deline } from "../util/string"
 import { BooleanParameter, StringsParameter } from "../cli/params"
 import { validateActionSearchResults, watchParameter, watchRemovedWarning } from "./helpers"
+import { Log } from "../logger/log-entry"
+import { TestCommand } from "./test"
+import { WorkflowCommand, WorkflowRunOutput } from "./workflow"
 
 // TODO: support interactive execution for a single Run (needs implementation from RunTask through plugin handlers).
 
@@ -103,15 +114,21 @@ export class RunCommand extends Command<Args, Opts> {
     printHeader(headerLog, msg, "🏃‍♂️")
   }
 
-  async action(params: CommandParams<Args, Opts>) {
+  async action(
+    params: CommandParams<Args, Opts>
+  ): Promise<CommandResult<ProcessCommandResult> | CommandResult<WorkflowRunOutput>> {
     const { garden, log, footerLog, args, opts } = params
 
     // Detect possible old-style invocations as early as possible
     // Needs to be done before graph init to support lazy init usecases, e.g. workflows
     let names: string[] | undefined = undefined
     if (args.names && args.names.length > 0) {
       names = args.names
-      detectOldRunCommand(names, args, opts)
+      const result = await maybeOldRunCommand(names, args, opts, log, params)
+      // If we get a result from the old-style compatibility runner, early return it instead of continuing
+      if (result) {
+        return result
+      }
     }
 
     if (opts.watch) {
@@ -195,29 +212,54 @@ export class RunCommand extends Command<Args, Opts> {
   }
 }
 
-function detectOldRunCommand(names: string[], args: any, opts: any) {
-  if (["module", "service", "task", "test", "workflow"].includes(names[0])) {
-    let renameDescription = ""
-    const firstArg = names[0]
+/**
+ * Helper function for detecting an old-style run command invocation and passing the params to the correct handlers. Examples: `garden run workflow foo`, `garden run test`.
+ *
+ * This is slightly hacky with any types and other parameter adjusting, but is required for backwards compatibility.
+ */
+function maybeOldRunCommand(names: string[], args: any, opts: any, log: Log, params: any) {
+  const firstArg = names[0]
+  if (["module", "service", "task", "test", "workflow"].includes(firstArg)) {
     if (firstArg === "module" || firstArg === "service") {
-      renameDescription = `The ${chalk.white("garden run " + firstArg)} command has been removed.
-      Please define a Run action instead, or use the underlying tools (e.g. Docker or Kubernetes) directly.`
+      throw new ParameterError(
+        `Error: The ${chalk.white("garden run " + firstArg)} command has been removed.
+      Please define a Run action instead, or use the underlying tools (e.g. Docker or Kubernetes) directly.`,
+        { args, opts }
+      )
     }
     if (firstArg === "task") {
-      renameDescription = `The ${chalk.yellow(
-        "run task"
-      )} command was removed in Garden 0.13. Please use the ${chalk.yellow("run")} command instead.`
+      log.warn(
+        `The ${chalk.yellow("run task")} command will be removed in Garden 0.14. Please use the ${chalk.yellow(
+          "run"
+        )} command instead.`
+      )
+      // Remove the `task` arg and continue execution in the Run handler
+      names.shift()
+      return
     }
     if (firstArg === "test") {
-      renameDescription = `The ${chalk.yellow(
-        "run test"
-      )} command was removed in Garden 0.13. Please use the ${chalk.yellow("test")} command instead.`
+      log.warn(
+        `The ${chalk.yellow("run test")} command will be removed in Garden 0.14. Please use the ${chalk.yellow(
+          "test"
+        )} command instead.`
+      )
+      // Remove the `test` arg and execute in the Test handler
+      names.shift()
+      const testCmd = new TestCommand()
+      return testCmd.action(params)
     }
     if (firstArg === "workflow") {
-      renameDescription = `The ${chalk.yellow(
-        "run workflow"
-      )} command was removed in Garden 0.13. Please use the ${chalk.yellow("workflow")} command instead.`
+      log.warn(
+        `The ${chalk.yellow("run workflow")} command will be removed in Garden 0.14. Please use the ${chalk.yellow(
+          "workflow"
+        )} command instead.`
+      )
+      // Remove the `workflow` arg and execute in the Workflow handler
+      names.shift()
+      const workflowCmd = new WorkflowCommand()
+      const workflow = names[0] // NOTE: the workflow command only supports passing one workflow name, not a list
+      return workflowCmd.action({ ...params, args: { ...args, workflow } })
     }
-    throw new ParameterError(`Error: ${renameDescription}`, { args, opts })
   }
+  return
 }
diff --git a/core/src/commands/workflow.ts b/core/src/commands/workflow.ts
@@ -48,7 +48,7 @@ const runWorkflowArgs = {
 
 type Args = typeof runWorkflowArgs
 
-interface WorkflowRunOutput {
+export interface WorkflowRunOutput {
   steps: { [stepName: string]: WorkflowStepResult }
 }
 

diff --git a/core/test/data/test-projects/old-style-run-invocations/garden.yml b/core/test/data/test-projects/old-style-run-invocations/garden.yml
@@ -0,0 +1,13 @@
+# TODO: remove this entire test project directory in 0.14
+apiVersion: garden.io/v0
+kind: Project
+name: test-project-a
+environments:
+  - name: local
+providers:
+  - name: test-plugin
+variables:
+  some: variable
+outputs:
+  - name: taskName
+    value: task-a
diff --git a/core/test/data/test-projects/old-style-run-invocations/module-a/garden.yml b/core/test/data/test-projects/old-style-run-invocations/module-a/garden.yml
@@ -0,0 +1,27 @@
+kind: Module
+name: module-a
+type: test
+variables:
+  msg: OK
+services:
+  - name: service-a
+build:
+  command: [echo, A]
+tests:
+  - name: unit
+    command: [echo, "${var.msg}"]
+  - name: integration
+    command: [echo, "${var.msg}"]
+    dependencies:
+      - service-a
+tasks:
+  - name: task-a
+    command: [echo, "${var.msg}"]
+  - name: task-a2
+    command: [echo, "${environment.name}-${var.msg}"]
+
+---
+kind: Workflow
+name: workflow-a
+steps:
+  - script: echo
diff --git a/core/test/unit/src/commands/run.ts b/core/test/unit/src/commands/run.ts
@@ -8,7 +8,16 @@
 
 import { expect } from "chai"
 import { RunCommand } from "../../../../src/commands/run"
-import { TestGarden, makeTestGardenA, expectError, getAllProcessedTaskNames } from "../../../helpers"
+import {
+  TestGarden,
+  expectError,
+  getAllProcessedTaskNames,
+  makeTestGarden,
+  getDataDir,
+  makeTestGardenA,
+} from "../../../helpers"
+import { getLogMessages } from "../../../../src/util/testing"
+import { LogLevel } from "../../../../src/logger/logger"
 
 // TODO-G2: fill in test implementations. use TestCommand tests for reference.
 
@@ -89,74 +98,6 @@ describe("RunCommand", () => {
     )
   })
 
-  context("detecting invocations to removed 0.12-era run subcommands", () => {
-    it("throws if called with 'test' as the first argument", async () => {
-      await expectError(
-        () =>
-          garden.runCommand({
-            command,
-            args: { names: ["test", "foo"] },
-            opts: {
-              "force": true,
-              "force-build": true,
-              "watch": false,
-              "skip": [],
-              "skip-dependencies": false,
-              "module": undefined,
-            },
-          }),
-        {
-          contains:
-            "The run test command was removed in Garden 0.13",
-        }
-      )
-    })
-
-    it("throws if called with 'task' as the first argument", async () => {
-      await expectError(
-        () =>
-          garden.runCommand({
-            command,
-            args: { names: ["task", "foo"] },
-            opts: {
-              "force": true,
-              "force-build": true,
-              "watch": false,
-              "skip": [],
-              "skip-dependencies": false,
-              "module": undefined,
-            },
-          }),
-        {
-          contains:
-            "The run task command was removed in Garden 0.13",
-        }
-      )
-    })
-
-    it("throws if called with 'workflow' as the first argument", async () => {
-      await expectError(
-        () =>
-          garden.runCommand({
-            command,
-            args: { names: ["workflow", "foo"] },
-            opts: {
-              "force": true,
-              "force-build": true,
-              "watch": false,
-              "skip": [],
-              "skip-dependencies": false,
-              "module": undefined,
-            },
-          }),
-        {
-          contains:
-            "The run workflow command was removed in Garden 0.13",
-        }
-      )
-    })
-  })
-
   it("supports '*' as an argument to select all Runs", async () => {
     const { result } = await garden.runCommand({
       command,
@@ -350,3 +291,75 @@ describe("RunCommand", () => {
     })
   })
 })
+
+// TODO: switch these back to expectError for 0.14 by reverting a commit from https://github.com/garden-io/garden/pull/4195
+describe("RunCommand legacy invocations", () => {
+  const command = new RunCommand()
+
+  let garden: TestGarden
+
+  beforeEach(async () => {
+    garden = await makeTestGarden(getDataDir("test-projects", "old-style-run-invocations"))
+  })
+
+  context("detecting invocations to removed 0.12-era run subcommands", async () => {
+    it("warns if called with 'test' as the first argument", async () => {
+      const log = garden.log
+      await garden.runCommand({
+        command,
+        args: { names: ["test", "module-a-unit"] },
+        opts: {
+          "force": true,
+          "force-build": true,
+          "watch": false,
+          "skip": [],
+          "skip-dependencies": false,
+          "module": undefined,
+        },
+      })
+      const logMessages = getLogMessages(log, (l) => l.level === LogLevel.warn)
+      expect(logMessages[0]).to.include("The run test command will be removed in Garden 0.14")
+    })
+
+    it("warns if called with 'task' as the first argument", async () => {
+      const log = garden.log
+      await garden.runCommand({
+        command,
+        args: { names: ["task", "task-a"] },
+        opts: {
+          "force": true,
+          "force-build": true,
+          "watch": false,
+          "skip": [],
+          "skip-dependencies": false,
+          "module": undefined,
+        },
+      })
+      const logMessages = getLogMessages(log, (l) => l.level === LogLevel.warn)
+      expect(logMessages[0]).to.include("The run task command will be removed in Garden 0.14")
+    })
+
+    it("warns if called with 'workflow' as the first argument", async () => {
+      const log = garden.log
+      try {
+        await garden.runCommand({
+          command,
+          args: { names: ["workflow", "workflow-a"] },
+          opts: {
+            "force": true,
+            "force-build": true,
+            "watch": false,
+            "skip": [],
+            "skip-dependencies": false,
+            "module": undefined,
+          },
+        })
+      } catch (e) {
+        // we get an output schema validation error here.
+        // ignoring it for test purposes - we're only interested in the warning
+      }
+      const logMessages = getLogMessages(log, (l) => l.level === LogLevel.warn)
+      expect(logMessages[0]).to.include("The run workflow command will be removed in Garden 0.14")
+    })
+  })
+})