fix(template): avoid premature disabled flag evaluation on actions (#…

…6448)

* fix(template): avoid premature `disabled` flag evaluation on actions

* Do early `disabled` flag evaluation only for actions with duplicate names.
* Deny the usage of `var` and `variables` contexts in early `disabled` flag evaluation.

The contexts `var` and `variables` contexts are not fully resolved at that stage.
That can lead to incorrectly resolved `disabled` flag values.

Co-authored-by: Steffen Neubauer <steffen@garden.io>

* refactor: narrow config type in `ActionConfigContextParams`

It can never be `WorkflowConfig`.

* test: amend the test data

* test: fix assertions

* fix: denied contexts definition

Co-authored-by: Steffen Neubauer <steffen@garden.io>

* test: update test assertion

Co-authored-by: Steffen Neubauer <steffen@garden.io>

---------

Co-authored-by: Steffen Neubauer <steffen@garden.io>

core/src/config/template-contexts/actions.ts

@@ -19,7 +19,6 @@ import { ConfigContext, ErrorContext, ParentContext, schema, TemplateContext } f
 import { exampleVersion, OutputConfigContext } from "./module.js"
 import { TemplatableConfigContext } from "./project.js"
 import { DOCS_BASE_URL } from "../../constants.js"
-import type { WorkflowConfig } from "../workflow.js"
 import { styles } from "../../logger/styles.js"
 
 function mergeVariables({ garden, variables }: { garden: Garden; variables: DeepPrimitiveMap }): DeepPrimitiveMap {
@@ -64,7 +63,7 @@ class ActionConfigThisContext extends ConfigContext {
 
 interface ActionConfigContextParams {
   garden: Garden
-  config: ActionConfig | WorkflowConfig
+  config: ActionConfig
   thisContextParams: ActionConfigThisContextParams
   variables: DeepPrimitiveMap
 }

core/src/garden.ts

@@ -1522,10 +1522,16 @@ export class Garden {
     }
 
     const context = new TemplatableConfigContext(this, config)
-    // Inject action's variables
-    context.variables = context.var = {
-      ...context.variables,
-      ...config.variables,
+    // Hack: deny variables contexts here, because those have not been fully resolved yet.
+    const deniedContexts = ["var", "variables"]
+    for (const deniedContext of deniedContexts) {
+      Object.defineProperty(context, deniedContext, {
+        get: () => {
+          throw new ConfigurationError({
+            message: `If you have duplicate action names, the ${styles.accent("`disabled`")} flag cannot depend on the ${styles.accent(`\`${deniedContext}\``)} context.`,
+          })
+        },
+      })
     }
 
     return resolveTemplateString({
@@ -1543,10 +1549,11 @@ export class Garden {
     const key = actionReferenceToString(config)
     const existing = this.actionConfigs[config.kind][config.name]
 
-    // Resolve the actual values of the `disabled` flag
-    config.disabled = this.evaluateDisabledFlag(config)
-
     if (existing) {
+      // Resolve the actual values of the `disabled` flag
+      config.disabled = this.evaluateDisabledFlag(config)
+      existing.disabled = this.evaluateDisabledFlag(existing)
+
       if (actionIsDisabled(config, this.environmentName)) {
         this.log.silly(
           () => `Skipping action ${key} because it is disabled and another action with the same key exists`

core/test/data/test-projects/disabled-action-with-var-context/project.garden.yml

@@ -0,0 +1,10 @@
+apiVersion: garden.io/v1
+kind: Project
+name: disabled-action-with-var-context
+defaultEnvironment: local
+environments:
+  - name: local
+  - name: remote
+providers:
+  - name: exec
+    environments: [ local, remote ]

core/test/data/test-projects/disabled-action-with-var-context/runs.garden.yml

@@ -0,0 +1,18 @@
+kind: Run
+name: run-script
+type: exec
+var:
+  foo: bar
+spec:
+  command: ["sh", "-c", "echo 'Hello from local'"]
+
+---
+
+kind: Run
+name: run-script
+type: exec
+var:
+  $merge: ${actions.run["run-script"].var}
+disabled: "${var.foo != 'bar'}"
+spec:
+  command: ["sh", "-c", "echo 'Hello from remote'"]

core/test/unit/src/garden.ts

@@ -3052,6 +3052,22 @@ describe("Garden", () => {
       expect(runScript.getConfig().spec.command).to.eql(["sh", "-c", "echo 'Hello from local'"])
     })
 
+    it("should deny variables context in disabled flag for actions with duplicate names", async () => {
+      const garden = await makeTestGarden(getDataDir("test-projects", "disabled-action-with-var-context"))
+
+      // There are 2 'run-script' actions defined in the project, one per environment.
+      await expectError(() => garden.getConfigGraph({ log: garden.log, emit: false }), {
+        contains: [
+          "If you have duplicate action names",
+          "the",
+          "disabled",
+          "flag cannot depend on the",
+          "var",
+          "context",
+        ],
+      })
+    })
+
     it("should resolve actions from templated config templates", async () => {
       const garden = await makeTestGarden(getDataDir("test-projects", "config-templates-with-templating"))
       await garden.scanAndAddConfigs()
@@ -3080,7 +3096,6 @@ describe("Garden", () => {
         kind: "Run",
         type: "exec",
         name: runNameA,
-        disabled: false,
         spec: {
           command: ["echo", runNameA],
         },
@@ -3093,7 +3108,6 @@ describe("Garden", () => {
         kind: "Run",
         type: "exec",
         name: runNameB,
-        disabled: false,
         spec: {
           command: ["echo", runNameB],
         },