Skip to content

Commit

Permalink
fix(publish): respect the spec.publishId in build action config (#5585
Browse files Browse the repository at this point in the history
)

* fix(publish): respect the `spec.publishId` in build action config

* test(publishContainerBuild): add more test cases and do not mock
getPublicImageId

* test: add test for PublishTask to pass undefined to plugin if no --tag
option was specified

* chore: fix double assignment

---------

Co-authored-by: Steffen Neubauer <[email protected]>
vvagaytsev and stefreak authored Dec 20, 2023
1 parent 6214366 commit f60cde7
Showing 9 changed files with 172 additions and 88 deletions.
3 changes: 1 addition & 2 deletions core/src/commands/publish.ts
Original file line number Diff line number Diff line change
@@ -99,8 +99,7 @@ export class PublishCommand extends Command<Args, Opts, ProcessCommandResult> {
log,
action,
forceBuild: opts["force-build"],
tagTemplate: opts.tag,

tagOverrideTemplate: opts.tag,
force: false,
})
})
5 changes: 4 additions & 1 deletion core/src/plugin/handlers/Build/publish.ts
Original file line number Diff line number Diff line change
@@ -16,7 +16,10 @@ import type { ActionStatus, Executed } from "../../../actions/types.js"
import { actionStatusSchema } from "../../../actions/base.js"

interface PublishActionParams<T extends BuildAction = BuildAction> extends PluginBuildActionParamsBase<T> {
tag?: string
/**
* This is only defined when a user defines --tag option.
*/
tagOverride?: string
}

export interface PublishActionDetail {
39 changes: 23 additions & 16 deletions core/src/plugins/container/helpers.ts
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@

import { join, posix } from "path"
import fsExtra from "fs-extra"

const { readFile, pathExists, lstat } = fsExtra
import semver from "semver"
import type { CommandEntry } from "docker-file-parser"
@@ -84,30 +85,36 @@ const helpers = {
* (not to be confused with the ID used when pushing to private deployment registries).
*
* The tag on the identifier will be set as one of (in order of precedence):
* - The `tag` variable explicitly set (e.g. set on the garden publish command).
* - The tag part of the `image` field, if one is set and it includes a tag part.
* - The `tagOverride` argument explicitly set (e.g. --tag option provided to the garden publish command).
* - The tag part of the `spec.publishId` from the action configuration, if one is set, and it includes a tag part.
* - The Garden version of the module.
*/
getPublicImageId(action: Resolved<ContainerBuildAction>, tag?: string) {
getPublicImageId(action: Resolved<ContainerBuildAction>, tagOverride?: string) {
// TODO: allow setting a default user/org prefix in the project/plugin config
const explicitImage = action.getSpec("publishId")
const explicitPublishId = action.getSpec("publishId")

if (explicitImage) {
let parsedImage: ParsedImageId
let publishTag = tagOverride

if (explicitPublishId) {
// Getting the tag like this because it's otherwise defaulted to "latest"
const imageTag = splitFirst(explicitImage, ":")[1]
const parsedImage = helpers.parseImageId(explicitImage)
if (!tag) {
tag = imageTag || action.versionString()
const imageTag = splitFirst(explicitPublishId, ":")[1]
if (!publishTag) {
publishTag = imageTag
}
return helpers.unparseImageId({ ...parsedImage, tag })

parsedImage = helpers.parseImageId(explicitPublishId)
} else {
const localImageName = action.name
const parsedImage = helpers.parseImageId(localImageName)
if (!tag) {
tag = action.versionString()
}
return helpers.unparseImageId({ ...parsedImage, tag })
const explicitImage = action.getSpec("localId")
const localImageName = this.getLocalImageName(action.name, explicitImage)

parsedImage = helpers.parseImageId(localImageName)
}

if (!publishTag) {
publishTag = action.versionString()
}
return helpers.unparseImageId({ ...parsedImage, tag: publishTag })
},

/**
4 changes: 2 additions & 2 deletions core/src/plugins/container/publish.ts
Original file line number Diff line number Diff line change
@@ -14,10 +14,10 @@ export const publishContainerBuild: BuildActionHandler<"publish", ContainerBuild
ctx,
action,
log,
tag,
tagOverride,
}) => {
const localId = action.getOutput("localImageId")
const remoteId = containerHelpers.getPublicImageId(action, tag)
const remoteId = containerHelpers.getPublicImageId(action, tagOverride)

if (localId !== remoteId) {
log.info({ msg: `Tagging image with ${localId} and ${remoteId}` })
4 changes: 2 additions & 2 deletions core/src/plugins/kubernetes/container/publish.ts
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@ import type { BuildActionHandler } from "../../../plugin/action-types.js"
import { containerHelpers } from "../../container/helpers.js"

export const k8sPublishContainerBuild: BuildActionHandler<"publish", ContainerBuildAction> = async (params) => {
const { ctx, action, log, tag } = params
const { ctx, action, log, tagOverride } = params
const k8sCtx = ctx as KubernetesPluginContext
const provider = k8sCtx.provider

@@ -36,7 +36,7 @@ export const k8sPublishContainerBuild: BuildActionHandler<"publish", ContainerBu
// optionally use the tag instead of the garden version, this requires that we tag the image locally
// before publishing to the remote registry

const remotePublishId = tag ? `${action.getOutput("deploymentImageName")}:${tag}` : remoteId
const remotePublishId = tagOverride ? `${action.getOutput("deploymentImageName")}:${tagOverride}` : remoteId

await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["tag", localId, remotePublishId], log, ctx })

27 changes: 18 additions & 9 deletions core/src/tasks/publish.ts
Original file line number Diff line number Diff line change
@@ -20,22 +20,28 @@ import { ActionSpecContext } from "../config/template-contexts/actions.js"
import { OtelTraced } from "../util/open-telemetry/decorators.js"

export interface PublishTaskParams extends BaseActionTaskParams<BuildAction> {
tagTemplate?: string
/**
* Only defined if --tag option is used in the garden publish command.
*/
tagOverrideTemplate?: string
}

export class PublishTask extends BaseActionTask<BuildAction, PublishActionResult> {
type = "publish"
override concurrencyLimit = 5

tagTemplate?: string
/**
* Only defined if --tag option is used in the garden publish command.
*/
tagOverrideTemplate?: string

constructor(params: PublishTaskParams) {
super(params)
this.tagTemplate = params.tagTemplate
this.tagOverrideTemplate = params.tagOverrideTemplate
}

protected override getDependencyParams(): PublishTaskParams {
return { ...super.getDependencyParams(), tagTemplate: this.tagTemplate }
return { ...super.getDependencyParams(), tagOverrideTemplate: this.tagOverrideTemplate }
}

override resolveStatusDependencies() {
@@ -89,9 +95,10 @@ export class PublishTask extends BaseActionTask<BuildAction, PublishActionResult
const action = this.getExecutedAction(this.action, dependencyResults)
const version = action.versionString()

let tag = version
// This is only defined when a user defines --tag option
let tagOverride: string | undefined = undefined

if (this.tagTemplate) {
if (this.tagOverrideTemplate) {
const resolvedProviders = await this.garden.resolveProviders(this.log)

const templateContext = new BuildTagContext({
@@ -107,18 +114,20 @@ export class PublishTask extends BaseActionTask<BuildAction, PublishActionResult
})

// Resolve template string and make sure the result is a string
tag = "" + resolveTemplateString({ string: this.tagTemplate, context: templateContext })
tagOverride = "" + resolveTemplateString({ string: this.tagOverrideTemplate, context: templateContext })

// TODO: validate the tag?
}

this.log.info("Publishing with tag " + tag)
if (tagOverride) {
this.log.info(`Publish tag has been overridden with the --tag command line option: ${tagOverride}`)
}

const router = await this.garden.getActionRouter()

let result: PublishActionResult
try {
const output = await router.build.publish({ action, log: this.log, graph: this.graph, tag })
const output = await router.build.publish({ action, log: this.log, graph: this.graph, tagOverride })
result = output.result
} catch (err) {
this.log.error(`Failed publishing build ${action.name}`)
118 changes: 82 additions & 36 deletions core/test/integ/src/plugins/container/container.ts
Original file line number Diff line number Diff line change
@@ -54,6 +54,8 @@ describe("plugins.container", () => {
let log: ActionLog
let containerProvider: ContainerProvider

let dockerCli: sinon.SinonStub<any, any>

beforeEach(async () => {
garden = await makeTestGarden(projectRoot, { plugins: [gardenPlugin()] })
log = createActionLog({ log: garden.log, actionName: "", actionKind: "" })
@@ -62,15 +64,23 @@ describe("plugins.container", () => {
td.replace(garden.buildStaging, "syncDependencyProducts", () => null)
})

afterEach(() => {
sinon.restore()
})

async function getTestBuild(cfg: BuildActionConfig): Promise<Executed<ContainerBuildAction>> {
sinon.replace(containerHelpers, "actionHasDockerfile", async () => true)
sinon.replace(containerHelpers, "dockerCli", async () => ({
all: "test log",
stdout: testVersionedId,
stderr: "",
code: 0,
proc: <any>null,
}))

dockerCli = sinon.stub(containerHelpers, "dockerCli")
dockerCli.returns(
Promise.resolve({
all: "test log",
stdout: testVersionedId,
stderr: "",
code: 0,
proc: <any>null,
})
)

garden.setActionConfigs([cfg])
const graph = await garden.getConfigGraph({ emit: false, log })
@@ -80,34 +90,6 @@ describe("plugins.container", () => {
}

describe("publishContainerBuild", () => {
it("should publish image", async () => {
const config = cloneDeep(baseConfig)
config.spec.localId = testVersionedId

sinon.replace(containerHelpers, "getPublicImageId", () => testVersionedId)

const action = await getTestBuild(config)

sinon.restore()

sinon.replace(containerHelpers, "getPublicImageId", () => testVersionedId)
sinon.replace(containerHelpers, "actionHasDockerfile", async () => true)

sinon.replace(containerHelpers, "dockerCli", async ({ cwd, args, ctx: _ctx }) => {
const out = { all: "log", stdout: "", stderr: "", code: 0, proc: <any>null }
if (args[0] === "tag") {
return out
}
expect(cwd).to.equal(action.getBuildPath())
expect(args).to.eql(["push", testVersionedId])
expect(_ctx).to.exist
return out
})

const result = await publishContainerBuild({ ctx, log, action })
expect(result.detail).to.eql({ message: "Published some/image:12345", published: true })
})

it("should tag image if remote id differs from local id", async () => {
const publishId = "some/image:1.1"

@@ -152,7 +134,7 @@ describe("plugins.container", () => {

const dockerCli = sinon.stub(containerHelpers, "dockerCli")

const result = await publishContainerBuild({ ctx, log, action, tag: "custom-tag" })
const result = await publishContainerBuild({ ctx, log, action, tagOverride: "custom-tag" })
expect(result.detail).to.eql({ message: "Published test:custom-tag", published: true })

sinon.assert.calledWith(
@@ -171,6 +153,70 @@ describe("plugins.container", () => {
})
)
})

describe("publish image id", () => {
let action: Executed<ContainerBuildAction>

function assertPublishId(publishId: string, detail: { message?: string; published: boolean } | null) {
expect(detail).to.eql({ message: `Published ${publishId}`, published: true })

sinon.assert.calledWith(
dockerCli,
sinon.match({
cwd: action.getBuildPath(),
args: ["push", publishId],
})
)
}

it("should use spec.publishId if defined", async () => {
const config = cloneDeep(baseConfig)
config.spec.publishId = testVersionedId

action = await getTestBuild(config)

const result = await publishContainerBuild({ ctx, log, action })
assertPublishId("some/image:12345", result.detail)
})

it("should fall back to spec.localId if spec.publishId is not defined", async () => {
const config = cloneDeep(baseConfig)
config.spec.localId = "private-registry/foobar"

action = await getTestBuild(config)

const result = await publishContainerBuild({ ctx, log, action })
assertPublishId(`private-registry/foobar:${action.versionString()}`, result.detail)
})

it("should fall back to action name if spec.localId and spec.publishId are not defined", async () => {
const config = cloneDeep(baseConfig)

action = await getTestBuild(config)

const result = await publishContainerBuild({ ctx, log, action })
assertPublishId(`test:${action.versionString()}`, result.detail)
})

it("should respect tagOverride, which corresponds to garden publish --tag command line option", async () => {
const config = cloneDeep(baseConfig)

action = await getTestBuild(config)

const result = await publishContainerBuild({ ctx, log, action, tagOverride: "custom-version" })
assertPublishId(`test:custom-version`, result.detail)
})

it("tagOverride has precedence over spec.publishId", async () => {
const config = cloneDeep(baseConfig)
config.spec.publishId = "some/image:1.1"

action = await getTestBuild(config)

const result = await publishContainerBuild({ ctx, log, action, tagOverride: "custom-version" })
assertPublishId(`some/image:custom-version`, result.detail)
})
})
})

describe("checkDockerServerVersion", () => {
12 changes: 6 additions & 6 deletions core/test/integ/src/plugins/kubernetes/container/build/build.ts
Original file line number Diff line number Diff line change
@@ -182,7 +182,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "foo",
tagOverride: "foo",
})

expect(result.detail?.message).to.eql(`Published ${remoteImageName}:foo`)
@@ -287,7 +287,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "0.1.0",
tagOverride: "0.1.0",
})

await executeBuild("private-base")
@@ -337,7 +337,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "foo",
tagOverride: "foo",
})

expect(result.detail?.message).to.eql(
@@ -427,7 +427,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "0.1.0",
tagOverride: "0.1.0",
})

await executeBuild("private-base")
@@ -487,7 +487,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "foo",
tagOverride: "foo",
})

expect(result.detail?.message).to.eql(
@@ -554,7 +554,7 @@ describe.skip("Kubernetes Container Build Extension", () => {
ctx,
action,
log,
tag: "0.1.0",
tagOverride: "0.1.0",
})

await executeBuild("private-base")
48 changes: 34 additions & 14 deletions core/test/unit/src/commands/publish.ts
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@ import { withDefaultGlobalOpts, makeTestGarden, getAllTaskResults, getDataDir }
import { taskResultOutputs } from "../../../helpers.js"
import cloneDeep from "fast-copy"

import type { PublishActionResult, PublishBuildAction } from "../../../../src/plugin/handlers/Build/publish.js"
import type { PublishBuildAction } from "../../../../src/plugin/handlers/Build/publish.js"
import type { GardenPluginSpec } from "../../../../src/plugin/plugin.js"
import { createGardenPlugin } from "../../../../src/plugin/plugin.js"
import type { ConvertModuleParams } from "../../../../src/plugin/handlers/Module/convert.js"
@@ -26,11 +26,6 @@ import { styles } from "../../../../src/logger/styles.js"
const projectRootB = getDataDir("test-project-b")

type PublishActionParams = ActionTypeHandlerParamsType<PublishBuildAction>
type PublishActionResultDetail = PublishActionResult["detail"]

const publishAction = async ({ tag }: PublishActionParams): Promise<PublishActionResultDetail> => {
return { published: true, identifier: tag }
}

const testProvider = createGardenPlugin({
name: "test-plugin",
@@ -72,10 +67,10 @@ const testProvider = createGardenPlugin({
docs: "Test plugin",
schema: joi.object().zodSchema(execBuildSpecSchema),
handlers: {
publish: async (params: PublishActionParams) => {
publish: async ({ tagOverride }: PublishActionParams) => {
return {
state: "ready",
detail: await publishAction(params),
detail: { published: true, identifier: tagOverride },
outputs: {},
}
},
@@ -124,6 +119,7 @@ describe("PublishCommand", () => {
const versionB = graph.getBuild("module-b").versionString()
const versionC = graph.getBuild("module-c").versionString()

// detail.identifier is undefined because --tag option was not specified; The plugin needs to calculate the tag itself, the test plugin will just return the tagOverride value.
expect(taskResultOutputs(result!)).to.eql({
"publish.module-a": {
outputs: {},
@@ -236,7 +232,6 @@ describe("PublishTask", () => {
it("should apply the specified tag to the published build", async () => {
const garden = await getTestGarden()
const log = garden.log
const tag = "foo"
const graph = await garden.getConfigGraph({ log, emit: true })
const builds = graph.getBuilds()

@@ -247,23 +242,22 @@ describe("PublishTask", () => {
log,
action,
forceBuild: false,
tagTemplate: tag,
tagOverrideTemplate: "foo",
force: false,
})
})

const processed = await garden.processTasks({ tasks, log, throwOnError: true })
const graphResultsMap = processed.results.getMap()
expect(graphResultsMap["publish.module-a"]!.result.detail.published).to.be.true
expect(graphResultsMap["publish.module-a"]!.result.detail.identifier).to.equal(tag)
expect(graphResultsMap["publish.module-a"]!.result.detail.identifier).to.equal("foo")
expect(graphResultsMap["publish.module-b"]!.result.detail.published).to.be.true
expect(graphResultsMap["publish.module-b"]!.result.detail.identifier).to.equal(tag)
expect(graphResultsMap["publish.module-b"]!.result.detail.identifier).to.equal("foo")
})

it("should resolve a templated tag and apply to the builds", async () => {
const garden = await getTestGarden()
const log = garden.log
const tag = "v1.0-${module.name}-${module.version}"

const graph = await garden.getConfigGraph({ log, emit: true })
const builds = graph.getBuilds()
@@ -275,7 +269,7 @@ describe("PublishTask", () => {
log,
action,
forceBuild: false,
tagTemplate: tag,
tagOverrideTemplate: "v1.0-${module.name}-${module.version}",
force: false,
})
})
@@ -290,4 +284,30 @@ describe("PublishTask", () => {
expect(graphResultsMap["publish.module-b"]!.result.detail.published).to.be.true
expect(graphResultsMap["publish.module-b"]!.result.detail.identifier).to.equal(`v1.0-module-b-${verB}`)
})

it("should pass tagOverride=undefined to plugin handler if tagOverride was undefined", async () => {
const garden = await getTestGarden()
const log = garden.log
const graph = await garden.getConfigGraph({ log, emit: true })
const builds = graph.getBuilds()

const tasks = builds.map((action) => {
return new PublishTask({
garden,
graph,
log,
action,
forceBuild: false,
tagOverrideTemplate: undefined,
force: false,
})
})

const processed = await garden.processTasks({ tasks, log, throwOnError: true })
const graphResultsMap = processed.results.getMap()
expect(graphResultsMap["publish.module-a"]!.result.detail.published).to.be.true
expect(graphResultsMap["publish.module-a"]!.result.detail.identifier).to.equal(undefined)
expect(graphResultsMap["publish.module-b"]!.result.detail.published).to.be.true
expect(graphResultsMap["publish.module-b"]!.result.detail.identifier).to.equal(undefined)
})
})

0 comments on commit f60cde7

Please sign in to comment.