Skip to content

Commit

Permalink
fix(module-conversion): always include build deps (#5671)
Browse files Browse the repository at this point in the history
* fix(module-conversion): always include build deps

Before this fix, build dependencies weren't being transferred to the
generated actions in all cases.

This was a problem e.g. if using a `helm` module with a build dependency
on a `container` module, whose built image was referenced in the chart
(since the missing build dependency meant that the image wasn't
necessarily ready by the time the deploy started).

This was fixed by simply ensuring that any explicit build dependencies
(i.e. under `module.build.dependencies`) are included in the
dependencies of any generated runtime actions (Deploys, Tests and Runs).

This reflects the 0.12-era semantics, where every test, service and task
had a build dependency on the module's build step, which in turn
respected any build dependencies declared on the module (under
`build.dependencies`).

* chore: remove unnecessary null-checker

* chore: extract some local vars and fix typo

* fix(container): don't add build dep on PVCs

In the module conversion logic for the `persistentvolumeclaim` type, we
don't generate any Build actions (just a single Deploy).

This means we shouldn't declare a build dependency for it (since we'd be
declaring a dependency on an action that won't exist after the
conversion).

* chore(test): add logging env vars for testing

Sometimes, it's convenient to see the logger output while debugging test
suites. By default, we use the `quiet` logger (which doesn't log
anything).

Here, we introduce two new env vars for testing,
`GARDEN_TESTS_SHOW_LOGS` and `GAREDEN_TESTS_LOG_LEVEL`.

* test: update test assertions

---------

Co-authored-by: Vladimir Vagaytsev <[email protected]>
  • Loading branch information
thsig and vvagaytsev authored Jan 30, 2024
1 parent a833eac commit 47c24d5
Show file tree
Hide file tree
Showing 6 changed files with 159 additions and 12 deletions.
3 changes: 1 addition & 2 deletions core/src/plugins/container/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ export async function configureContainerModule({ log, moduleConfig }: ConfigureM

for (const volume of spec.volumes) {
if (volume.module) {
moduleConfig.build.dependencies.push({ name: volume.module, copy: [] })
spec.dependencies.push(volume.module)
}
}
Expand Down Expand Up @@ -328,7 +327,7 @@ export async function convertContainerModule(params: ConvertModuleParams<Contain
)
actions.push(...runtimeActions)
if (buildAction) {
buildAction.dependencies = buildAction?.dependencies?.filter((d) => !volumeModulesReferenced.includes(d.name))
buildAction.dependencies = buildAction.dependencies?.filter((d) => !volumeModulesReferenced.includes(d.name))
}

return {
Expand Down
8 changes: 5 additions & 3 deletions core/src/plugins/container/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,8 @@ const helpers = {
// If we explicitly set a Dockerfile, we take that to mean you want it to be built.
// If the file turns out to be missing, this will come up in the build handler.

if (!!config.spec.dockerfile) {
const dockerfile = config.spec.dockerfile
if (!!dockerfile) {
return true
}

Expand All @@ -402,8 +403,9 @@ const helpers = {
// That's because the `image` field has the following two meanings:
// 1. Build an image with this name, if a Dockerfile exists
// 2. Deploy this image from the registry, if no Dockerfile exists
// This means we need to know if the Dockerfile exists before we know wether or not the Dockerfile will be present at runtime.
return version.files.includes(getDockerfilePath(config.path, config.spec.dockerfile))
// This means we need to know if the Dockerfile exists before we know whether the Dockerfile will be present at runtime.
const dockerfilePath = getDockerfilePath(config.path, dockerfile)
return version.files.includes(dockerfilePath)
},

async actionHasDockerfile(action: Resolved<ContainerBuildAction>): Promise<boolean> {
Expand Down
9 changes: 7 additions & 2 deletions core/src/resolve-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -796,9 +796,14 @@ export const convertModules = profileAsync(async function convertModules(
},

convertRuntimeDependencies,
// Note: We include any build dependencies from the module, since not all conversions generate a non-dummy
// build action (and we need to make sure build dependencies from the module are processed before the generated
// Deploy/Test/Run is).
prepareRuntimeDependencies(deps: string[], build?: BuildActionConfig<string, any>) {
const resolved: ActionReference[] = convertRuntimeDependencies(deps)
if (build) {
const buildDeps: ActionReference[] = module.build.dependencies.map(convertBuildDependency)
const resolved: ActionReference[] = [...buildDeps, ...convertRuntimeDependencies(deps)]
if (build && !buildDeps.find((d) => d.name === build.name && d.kind === "Build")) {
// We make sure not to add the same dependency twice here.
resolved.push({ kind: "Build", name: build.name })
}
return resolved
Expand Down
10 changes: 7 additions & 3 deletions core/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import type { SuiteFunction, TestFunction } from "mocha"
import type { AnalyticsGlobalConfig } from "../src/config-store/global.js"
import type { EventLogEntry, TestGardenOpts } from "../src/util/testing.js"
import { TestGarden } from "../src/util/testing.js"
import { LogLevel, RootLogger } from "../src/logger/logger.js"
import { LogLevel, RootLogger, parseLogLevel } from "../src/logger/logger.js"
import type { GardenCli } from "../src/cli/cli.js"
import { profileAsync } from "../src/util/profiling.js"
import { defaultDotIgnoreFile, makeTempDir } from "../src/util/fs.js"
Expand Down Expand Up @@ -523,11 +523,15 @@ export function findNamespaceStatusEvent(eventLog: EventLogEntry[], namespaceNam
*/
export function initTestLogger() {
// make sure logger is initialized
// Set GARDEN_TEST_SHOW_LOGS=true to see logger output when running tests.
const displayWriterType = process.env.GARDEN_TESTS_SHOW_LOGS ? "basic" : "quiet"
const logLevelFromEnv = process.env.GARDEN_TESTS_LOG_LEVEL
const logLevel = logLevelFromEnv ? parseLogLevel(logLevelFromEnv) : LogLevel.info
try {
RootLogger.initialize({
level: LogLevel.info,
level: <LogLevel>logLevel,
storeEntries: true,
displayWriterType: "quiet",
displayWriterType,
force: true,
})
} catch (_) {}
Expand Down
4 changes: 2 additions & 2 deletions core/test/unit/src/plugins/container/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ describe("plugins.container", () => {
})
})

it("should add service volume modules as build and runtime dependencies", async () => {
it("should add service volume modules as runtime dependencies and not as build ones", async () => {
const moduleConfig: ContainerModuleConfig = {
allowPublish: false,
build: { dependencies: [], timeout: DEFAULT_BUILD_TIMEOUT_SEC },
Expand Down Expand Up @@ -660,7 +660,7 @@ describe("plugins.container", () => {

const result = await configureContainerModule({ ctx, moduleConfig, log })

expect(result.moduleConfig.build.dependencies).to.eql([{ name: "volume-module", copy: [] }])
expect(result.moduleConfig.build.dependencies).to.eql([])
expect(result.moduleConfig.serviceConfigs[0].dependencies).to.eql(["volume-module"])
})

Expand Down
137 changes: 137 additions & 0 deletions core/test/unit/src/plugins/kubernetes/container/module-conversion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
* Copyright (C) 2018-2023 Garden Technologies, Inc. <[email protected]>
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

import { expect } from "chai"
import type { ConfigGraph } from "../../../../../../src/graph/config-graph.js"
import type { TempDirectory, TestGarden } from "../../../../../helpers.js"
import { makeModuleConfig, makeTempGarden } from "../../../../../helpers.js"
import type { Log } from "../../../../../../src/logger/log-entry.js"
import { gardenPlugin } from "../../../../../../src/plugins/container/container.js"
import type {
ContainerModuleConfig,
ContainerServiceSpec,
} from "../../../../../../src/plugins/container/moduleConfig.js"
import { defaultContainerResources } from "../../../../../../src/plugins/container/moduleConfig.js"
import { actionReferenceToString } from "../../../../../../src/actions/base.js"

describe("kubernetes container module conversion", () => {
let tmpDir: TempDirectory
let garden: TestGarden
let log: Log
let graph: ConfigGraph

before(async () => {
const result = await makeTempGarden({ plugins: [gardenPlugin()] })
tmpDir = result.tmpDir
garden = result.garden
log = garden.log
})

after(async () => {
tmpDir.cleanup()
})

it("should ", async () => {
garden.setModuleConfigs([
makeModuleConfig<ContainerModuleConfig>(garden.projectRoot, {
name: "test-image",
type: "container",
variables: {},
spec: {
build: {
timeout: 300,
},
buildArgs: {},
extraFlags: [],
dockerfile: "foo.dockerfile",
services: [],
tests: [],
tasks: [],
},
}),
makeModuleConfig<ContainerModuleConfig>(garden.projectRoot, {
name: "test-deploy",
type: "container",
variables: {},
build: {
timeout: 300,
dependencies: [
{
name: "test-image",
copy: [],
},
],
},
spec: {
build: {
timeout: 300,
},
buildArgs: {},
extraFlags: [],
dockerfile: "foo.dockerfile",
services: [
{
...dummyContainerServiceSpec,
name: "test-deploy",
},
],
tests: [],
tasks: [],
},
}),
])
graph = await garden.getConfigGraph({ log, emit: false })
const testDeploy = graph.getDeploy("test-deploy")
const deployDeps = testDeploy.getDependencyReferences().map(actionReferenceToString)
expect(deployDeps.sort()).to.eql(["build.test-deploy", "build.test-image"])
})
})

const dummyContainerServiceSpec: ContainerServiceSpec = {
name: "service-a",
annotations: {},
args: ["echo"],
dependencies: [],
daemon: false,
disabled: false,
ingresses: [
{
annotations: {},
path: "/",
port: "http",
},
],
env: {
SOME_ENV_VAR: "value",
},
healthCheck: {
httpGet: {
path: "/health",
port: "http",
},
livenessTimeoutSeconds: 10,
readinessTimeoutSeconds: 10,
},
limits: {
cpu: 123,
memory: 456,
},
cpu: defaultContainerResources.cpu,
memory: defaultContainerResources.memory,
ports: [
{
name: "http",
protocol: "TCP",
containerPort: 8080,
servicePort: 8080,
},
],
replicas: 1,
volumes: [],
deploymentStrategy: "RollingUpdate",
}

0 comments on commit 47c24d5

Please sign in to comment.