Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/config/template-contexts/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -64,7 +63,7 @@ class ActionConfigThisContext extends ConfigContext {

interface ActionConfigContextParams {
garden: Garden
config: ActionConfig | WorkflowConfig
config: ActionConfig
thisContextParams: ActionConfigThisContextParams
variables: DeepPrimitiveMap
}
Expand Down
21 changes: 14 additions & 7 deletions core/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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`
Expand Down
Original file line number Diff line number Diff line change
@@ -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 ]
Original file line number Diff line number Diff line change
@@ -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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if I use actions context directly in the disabled flag? Should we deny that as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already denied, the error will be thrown:

Invalid template string (...) at path disabled: Could not find key actions. Available keys: command, datetime, environment, git, inputs, local, parent, project, secrets, template, this, var and variables.

disabled: "${var.foo != 'bar'}"
spec:
command: ["sh", "-c", "echo 'Hello from remote'"]
vvagaytsev marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 16 additions & 2 deletions core/test/unit/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -3080,7 +3096,6 @@ describe("Garden", () => {
kind: "Run",
type: "exec",
name: runNameA,
disabled: false,
spec: {
command: ["echo", runNameA],
},
Expand All @@ -3093,7 +3108,6 @@ describe("Garden", () => {
kind: "Run",
type: "exec",
name: runNameB,
disabled: false,
spec: {
command: ["echo", runNameB],
},
Expand Down