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

feat(config): allow multiple actions with same key if all but one is disabled #4805

Merged
merged 1 commit into from
Jul 11, 2023
Merged
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
8 changes: 7 additions & 1 deletion core/src/actions/base.ts
Original file line number Diff line number Diff line change
@@ -405,7 +405,8 @@ export abstract class BaseAction<

isDisabled(): boolean {
// TODO: return true if group is disabled
return !!this.getConfig("disabled")
// TODO: implement environments field on action config
return actionIsDisabled(this._config, "TODO")
}

/**
@@ -876,3 +877,8 @@ export function addActionDependency(dep: ActionDependency, dependencies: ActionD
}
dependencies.push(dep)
}

export function actionIsDisabled(config: ActionConfig, _environmentName: string): boolean {
// TODO: implement environment fields and check if environment is disabled
return config.disabled === true
}
33 changes: 20 additions & 13 deletions core/src/garden.ts
Original file line number Diff line number Diff line change
@@ -137,7 +137,7 @@ import {
ActionModeMap,
BaseActionConfig,
} from "./actions/types"
import { actionReferenceToString, isActionConfig } from "./actions/base"
import { actionIsDisabled, actionReferenceToString, isActionConfig } from "./actions/base"
import { GraphSolver, SolveOpts, SolveParams, SolveResult } from "./graph/solver"
import { actionConfigsToGraph, actionFromConfig, executeAction, resolveAction, resolveActions } from "./graph/actions"
import { ActionTypeDefinition } from "./plugin/action-types"
@@ -1019,6 +1019,7 @@ export class Garden {
moduleGraph,
actionModes,
linkedSources,
environmentName: this.environmentName,
})

// TODO-0.13.1: detect overlap on Build actions
@@ -1388,22 +1389,28 @@ export class Garden {
*/
protected addActionConfig(config: BaseActionConfig) {
this.log.silly(`Adding ${config.kind} action ${config.name}`)
const key = actionReferenceToString(config)
const existing = this.actionConfigs[config.kind][config.name]

if (existing) {
const paths = [
existing.internal.configFilePath || existing.internal.basePath,
config.internal.configFilePath || config.internal.basePath,
]
const [pathA, pathB] = paths.map((path) => relative(this.projectRoot, path)).sort()
if (actionIsDisabled(config, this.environmentName)) {
this.log.silly(`Skipping action ${key} because it is disabled and another action with the same key exists`)
return
} else if (!actionIsDisabled(existing, this.environmentName)) {
const paths = [
existing.internal.configFilePath || existing.internal.basePath,
config.internal.configFilePath || config.internal.basePath,
]
const [pathA, pathB] = paths.map((path) => relative(this.projectRoot, path)).sort()

throw new ConfigurationError({
message: `${config.kind} action ${config.name} is declared multiple times (in '${pathA}' and '${pathB}')`,
detail: {
pathA,
pathB,
},
})
throw new ConfigurationError({
message: `${config.kind} action ${config.name} is declared multiple times (in '${pathA}' and '${pathB}') and neither is disabled.`,
detail: {
pathA,
pathB,
},
})
}
}

this.actionConfigs[config.kind][config.name] = config
18 changes: 15 additions & 3 deletions core/src/graph/actions.ts
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ import { relative } from "path"
import { profileAsync } from "../util/profiling"
import { uuidv4 } from "../util/random"
import { getConfigBasePath } from "../vcs/vcs"
import { actionIsDisabled } from "../actions/base"

export const actionConfigsToGraph = profileAsync(async function actionConfigsToGraph({
garden,
@@ -74,6 +75,7 @@ export const actionConfigsToGraph = profileAsync(async function actionConfigsToG
moduleGraph,
actionModes,
linkedSources,
environmentName,
}: {
garden: Garden
log: Log
@@ -82,6 +84,7 @@ export const actionConfigsToGraph = profileAsync(async function actionConfigsToG
moduleGraph: ModuleGraph
actionModes: ActionModeMap
linkedSources: LinkedSourceMap
environmentName: string
}): Promise<MutableConfigGraph> {
const configsByKey: ActionConfigsByKey = {}

@@ -94,7 +97,16 @@ export const actionConfigsToGraph = profileAsync(async function actionConfigsToG
const existing = configsByKey[key]

if (existing) {
throw actionNameConflictError(existing, config, garden.projectRoot)
if (actionIsDisabled(config, environmentName)) {
log.silly(`Skipping disabled action ${key} in favor of other action with same key`)
return
} else if (actionIsDisabled(existing, environmentName)) {
log.silly(`Skipping disabled action ${key} in favor of other action with same key`)
configsByKey[key] = config
return
} else {
throw actionNameConflictError(existing, config, garden.projectRoot)
}
}
configsByKey[key] = config
}
@@ -344,10 +356,10 @@ export const actionFromConfig = profileAsync(async function actionFromConfig({
export function actionNameConflictError(configA: ActionConfig, configB: ActionConfig, rootPath: string) {
return new ConfigurationError({
message: dedent`
Found two actions of the same name and kind:
Found two actions of the same name and kind (and neither is disabled):
- ${describeActionConfigWithPath(configA, rootPath)}
- ${describeActionConfigWithPath(configB, rootPath)}
Please rename one of the two to avoid the conflict.
Please rename or disable one of the two to avoid the conflict.
`,
detail: { configA, configB },
})
Loading