fix: garden publish command to respect publishId (#6052)

* refactor: rename local vars for more clarity

* chore: log image tags while publishing

* chore: (tidying) re-arrange the code

To see more similarities between 2 handlers

* fix: read image tag from existing parsed image

The parsed image comes from a helper function.
That helpers function uses `splitLast` instead of `splitFirst`
with `:`-delimiter to get the tag name. That looks correct.

* chore: rename some local vars for more clarity

* chore: optimized imports

* refactor: inline local var

* refactor: extract helper function to reduce code duplication

The only diff between `getContainerBuildActionOutputs` and `k8sGetContainerBuildActionOutputs`
was in the deploymentImageId calculation that depended
on the presence of deployment registry config.

* chore: remove outdated comment

* refactor: rename local vars for more clarity

* refactor: rename functions args for more clarity

* chore: add clarifying comment

* refactor: unwrap else-conditions

* chore: always tag container images

Align the behaviour with the `k8sPublishContainerBuild`
which does not compare local and remote images.

The repeated tagging is idemponent and should not be too costly.

* refactor: inline function

It was used only in one place.

* chore: amend var names, logs and comments for better clarity

* chore: log Docker tags while local build

* refactor: declare semantically different image ids separately

Each in its own variable.

* fix: use correct Docker image ID in `publish` command

* fix: use local image name for deployment registries

Publish ID is used only by `publish` command,
and it should not be used for internal deployment registries
and while Build-actions resolution.

* improvement: add image details to outputs of `publish` command

* chore: delete outdated comment

core/src/plugins/container/build.ts

@@ -16,7 +16,6 @@ import { defaultDockerfileName } from "./config.js"
 import { joinWithPosix } from "../../util/fs.js"
 import type { Resolved } from "../../actions/types.js"
 import dedent from "dedent"
-import { splitFirst } from "../../util/string.js"
 import {
   CONTAINER_BUILD_CONCURRENCY_LIMIT_CLOUD_BUILDER,
   CONTAINER_BUILD_CONCURRENCY_LIMIT_LOCAL,
@@ -210,37 +209,7 @@ async function buildContainerInCloudBuilder(params: {
 }
 
 export function getContainerBuildActionOutputs(action: Resolved<ContainerBuildAction>): ContainerBuildOutputs {
-  const buildName = action.name
-  const localId = action.getSpec("localId")
-  const explicitImage = action.getSpec("publishId")
-  let imageId = localId
-  if (explicitImage) {
-    // override imageId if publishId is set
-    const imageTag = splitFirst(explicitImage, ":")[1]
-    const parsedImage = containerHelpers.parseImageId(explicitImage)
-    const tag = imageTag || action.versionString()
-    imageId = containerHelpers.unparseImageId({ ...parsedImage, tag })
-  }
-  const version = action.moduleVersion()
-
-  const localImageName = containerHelpers.getLocalImageName(buildName, localId)
-  const localImageId = containerHelpers.getLocalImageId(buildName, localId, version)
-
-  // Note: The deployment image name/ID outputs are overridden by the kubernetes provider, these defaults are
-  // generally not used.
-  const deploymentImageName = containerHelpers.getDeploymentImageName(buildName, imageId, undefined)
-  const deploymentImageId = containerHelpers.getBuildDeploymentImageId(buildName, imageId, version, undefined)
-
-  return {
-    localImageName,
-    localImageId,
-    deploymentImageName,
-    deploymentImageId,
-    "local-image-name": localImageName,
-    "local-image-id": localImageId,
-    "deployment-image-name": deploymentImageName,
-    "deployment-image-id": deploymentImageId,
-  }
+  return containerHelpers.getBuildActionOutputs(action, undefined)
 }
 
 export function getDockerBuildFlags(

core/src/plugins/container/helpers.ts

@@ -8,24 +8,22 @@
 
 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"
 import { parse } from "docker-file-parser"
 import isGlob from "is-glob"
 import { ConfigurationError, GardenError, RuntimeError } from "../../exceptions.js"
 import type { SpawnOutput } from "../../util/util.js"
 import { spawn } from "../../util/util.js"
-import type { ContainerRegistryConfig, ContainerModuleConfig } from "./moduleConfig.js"
-import { defaultTag as _defaultTag, defaultImageNamespace } from "./moduleConfig.js"
+import type { ContainerBuildOutputs, ContainerModuleConfig, ContainerRegistryConfig } from "./moduleConfig.js"
+import { defaultImageNamespace, defaultTag as _defaultTag } from "./moduleConfig.js"
 import type { Writable } from "stream"
-import { flatten, uniq, fromPairs, reduce } from "lodash-es"
+import { flatten, fromPairs, reduce, uniq } from "lodash-es"
 import type { ActionLog, Log } from "../../logger/log-entry.js"
 
 import isUrl from "is-url"
 import titleize from "titleize"
-import { deline, stripQuotes, splitLast, splitFirst } from "../../util/string.js"
+import { deline, splitFirst, splitLast, stripQuotes } from "../../util/string.js"
 import type { PluginContext } from "../../plugin-context.js"
 import type { ModuleVersion } from "../../vcs/vcs.js"
 import type { ContainerBuildAction } from "./config.js"
@@ -36,6 +34,8 @@ import pMemoize from "../../lib/p-memoize.js"
 import { styles } from "../../logger/styles.js"
 import type { ContainerProviderConfig } from "./container.js"
 
+const { readFile, pathExists, lstat } = fsExtra
+
 interface DockerVersion {
   client?: string
   server?: string
@@ -60,9 +60,9 @@ const helpers = {
    * Returns the image ID used locally, when building and deploying to local environments
    * (when we don't need to push to remote registries).
    */
-  getLocalImageId(buildName: string, explicitImage: string | undefined, version: ModuleVersion): string {
+  getLocalImageId(buildName: string, explicitImageId: string | undefined, version: ModuleVersion): string {
     const { versionString } = version
-    const name = helpers.getLocalImageName(buildName, explicitImage)
+    const name = helpers.getLocalImageName(buildName, explicitImageId)
     const parsedImage = helpers.parseImageId(name)
     return helpers.unparseImageId({ ...parsedImage, tag: versionString })
   },
@@ -71,9 +71,9 @@ const helpers = {
    * Returns the image name used locally (without tag/version), when building and deploying to local environments
    * (when we don't need to push to remote registries).
    */
-  getLocalImageName(buildName: string, explicitImage: string | undefined): string {
-    if (explicitImage) {
-      const parsedImage = helpers.parseImageId(explicitImage)
+  getLocalImageName(buildName: string, explicitImageId: string | undefined): string {
+    if (explicitImageId) {
+      const parsedImage = helpers.parseImageId(explicitImageId)
       return helpers.unparseImageId({ ...parsedImage, tag: undefined })
     } else {
       return buildName
@@ -122,33 +122,33 @@ const helpers = {
    */
   getDeploymentImageName(
     buildName: string,
-    explicitImage: string | undefined,
+    explicitImageId: string | undefined,
     registryConfig: ContainerRegistryConfig | undefined
   ) {
-    const localName = explicitImage || buildName
-    const parsedId = helpers.parseImageId(localName)
-    const withoutVersion = helpers.unparseImageId({
-      ...parsedId,
-      tag: undefined,
-    })
+    const localImageName = explicitImageId || buildName
+    const parsedImageId = helpers.parseImageId(localImageName)
 
     if (!registryConfig) {
-      return withoutVersion
+      return helpers.unparseImageId({
+        ...parsedImageId,
+        tag: undefined,
+      })
     }
 
     const host = registryConfig.port ? `${registryConfig.hostname}:${registryConfig.port}` : registryConfig.hostname
 
     return helpers.unparseImageId({
       host,
       namespace: registryConfig.namespace,
-      repository: parsedId.repository,
+      repository: parsedImageId.repository,
       tag: undefined,
     })
   },
 
   /**
-   * Returns the image ID to be used when pushing to deployment registries. This always has the version
-   * set as the tag.
+   * Returns the image ID to be used when pushing to deployment registries.
+   * This always has the version set as the tag.
+   * Do not confuse this with the publishing image ID used by the `garden publish` command.
    */
   getBuildDeploymentImageId(
     buildName: string,
@@ -171,15 +171,48 @@ const helpers = {
     // Requiring this parameter to avoid accidentally missing it
     registryConfig: ContainerRegistryConfig | undefined
   ): string {
+    // The `dockerfile` configuration always takes precedence over the `image`.
     if (helpers.moduleHasDockerfile(moduleConfig, version)) {
       return helpers.getBuildDeploymentImageId(moduleConfig.name, moduleConfig.spec.image, version, registryConfig)
-    } else if (moduleConfig.spec.image) {
-      // Otherwise, return the configured image ID.
+    }
+
+    // Return the configured image ID if no Dockerfile is defined in the config.
+    if (moduleConfig.spec.image) {
       return moduleConfig.spec.image
-    } else {
-      throw new ConfigurationError({
-        message: `Module ${moduleConfig.name} neither specifies image nor can a Dockerfile be found in the module directory.`,
-      })
+    }
+
+    throw new ConfigurationError({
+      message: `Module ${moduleConfig.name} neither specifies image nor can a Dockerfile be found in the module directory.`,
+    })
+  },
+
+  /**
+   * Serves build action outputs in container and kubernetes plugins.
+   */
+  getBuildActionOutputs(
+    action: Resolved<ContainerBuildAction>,
+    // Requiring this parameter to avoid accidentally missing it
+    registryConfig: ContainerRegistryConfig | undefined
+  ): ContainerBuildOutputs {
+    const localId = action.getSpec("localId")
+    const version = action.moduleVersion()
+    const buildName = action.name
+
+    const localImageName = containerHelpers.getLocalImageName(buildName, localId)
+    const localImageId = containerHelpers.getLocalImageId(buildName, localId, version)
+
+    const deploymentImageName = containerHelpers.getDeploymentImageName(buildName, localId, registryConfig)
+    const deploymentImageId = containerHelpers.getBuildDeploymentImageId(buildName, localId, version, registryConfig)
+
+    return {
+      localImageName,
+      localImageId,
+      deploymentImageName,
+      deploymentImageId,
+      "local-image-name": localImageName,
+      "local-image-id": localImageId,
+      "deployment-image-name": deploymentImageName,
+      "deployment-image-id": deploymentImageId,
     }
   },
 

core/src/plugins/container/publish.ts

@@ -9,34 +9,36 @@
 import type { ContainerBuildAction } from "./moduleConfig.js"
 import { containerHelpers } from "./helpers.js"
 import type { BuildActionHandler } from "../../plugin/action-types.js"
+import { naturalList } from "../../util/string.js"
 
 export const publishContainerBuild: BuildActionHandler<"publish", ContainerBuildAction> = async ({
   ctx,
   action,
   log,
   tagOverride,
 }) => {
-  const localId = action.getOutput("localImageId")
-  const remoteId = containerHelpers.getPublicImageId(action, tagOverride)
+  const localImageId = action.getOutput("localImageId")
+  const remoteImageId = containerHelpers.getPublicImageId(action, tagOverride)
 
-  if (localId !== remoteId) {
-    log.info({ msg: `Tagging image with ${localId} and ${remoteId}` })
-    await containerHelpers.dockerCli({
-      cwd: action.getBuildPath(),
-      args: ["tag", localId, remoteId],
-      log,
-      ctx,
-    })
-  }
+  const taggedImages = [localImageId, remoteImageId]
+  log.info({ msg: `Tagging images ${naturalList(taggedImages)}` })
+  await containerHelpers.dockerCli({
+    cwd: action.getBuildPath(),
+    args: ["tag", ...taggedImages],
+    log,
+    ctx,
+  })
 
-  log.info({ msg: `Publishing image ${remoteId}...` })
+  log.info({ msg: `Publishing image ${remoteImageId}...` })
   // TODO: stream output to log if at debug log level
-  await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remoteId], log, ctx })
+  await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remoteImageId], log, ctx })
 
   return {
     state: "ready",
-    detail: { published: true, message: `Published ${remoteId}` },
-    // TODO-0.13.1
-    outputs: {},
+    detail: { published: true, message: `Published ${remoteImageId}` },
+    outputs: {
+      localImageId,
+      remoteImageId,
+    },
   }
 }

core/src/plugins/kubernetes/commands/pull-image.ts

@@ -9,12 +9,11 @@
 import fs from "fs"
 import tmp from "tmp-promise"
 import type { KubernetesPluginContext } from "../config.js"
-import { PluginError, ParameterError, GardenError } from "../../../exceptions.js"
+import { GardenError, ParameterError, PluginError, RuntimeError } from "../../../exceptions.js"
 import type { PluginCommand } from "../../../plugin/command.js"
 import { KubeApi } from "../api.js"
 import type { Log } from "../../../logger/log-entry.js"
 import { containerHelpers } from "../../container/helpers.js"
-import { RuntimeError } from "../../../exceptions.js"
 import { PodRunner } from "../run.js"
 import { dockerAuthSecretKey, getK8sUtilImageName, systemDockerAuthSecretName } from "../constants.js"
 import { getAppNamespace, getSystemNamespace } from "../namespace.js"
@@ -91,10 +90,7 @@ interface PullParams {
 }
 
 export async function pullBuild(params: PullParams) {
-  await pullFromExternalRegistry(params)
-}
-
-async function pullFromExternalRegistry({ ctx, log, localId, remoteId }: PullParams) {
+  const { ctx, log, localId, remoteId }: PullParams = params
   const api = await KubeApi.factory(log, ctx, ctx.provider)
   const buildMode = ctx.provider.config.buildMode
 

core/src/plugins/kubernetes/container/build/local.ts

@@ -18,6 +18,7 @@ import type { ContainerBuildAction } from "../../../container/moduleConfig.js"
 import type { BuildActionParams } from "../../../../plugin/action-types.js"
 import { k8sGetContainerBuildActionOutputs } from "../handlers.js"
 import { cloudBuilder } from "../../../container/cloudbuilder.js"
+import { naturalList } from "../../../../util/string.js"
 
 export const getLocalBuildStatus: BuildStatusHandler = async (params) => {
   const { ctx, action, log } = params
@@ -92,10 +93,13 @@ export const localBuild: BuildHandler = async (params) => {
 
   // Cloud Builder already pushes the image.
   if (!builtByCloudBuilder) {
-    log.info({ msg: `→ Pushing image ${remoteId} to remote...` })
-
     const buildPath = action.getBuildPath()
+    const taggedImages = [localId, remoteId]
+
+    log.info({ msg: `→ Tagging images ${naturalList(taggedImages)}` })
     await containerHelpers.dockerCli({ cwd: buildPath, args: ["tag", localId, remoteId], log, ctx })
+
+    log.info({ msg: `→ Pushing image ${remoteId} to remote...` })
     await containerHelpers.dockerCli({ cwd: buildPath, args: ["push", remoteId], log, ctx })
   }
 

core/src/plugins/kubernetes/container/handlers.ts

@@ -19,9 +19,7 @@ import type {
 import type { GetModuleOutputsParams } from "../../../plugin/handlers/Module/get-outputs.js"
 import { containerHelpers } from "../../container/helpers.js"
 import { getContainerModuleOutputs } from "../../container/container.js"
-import { getContainerBuildActionOutputs } from "../../container/build.js"
 import type { Resolved } from "../../../actions/types.js"
-import { splitFirst } from "../../../util/string.js"
 
 async function configure(params: ConfigureModuleParams<ContainerModule>) {
   const { moduleConfig } = await params.base!(params)
@@ -61,31 +59,7 @@ export function k8sGetContainerBuildActionOutputs({
   provider: KubernetesProvider
   action: Resolved<ContainerBuildAction>
 }): ContainerBuildOutputs {
-  const localId = action.getSpec("localId")
-  const outputs = getContainerBuildActionOutputs(action)
-  const explicitImage = action.getSpec("publishId")
-  let imageId = localId
-  if (explicitImage) {
-    // override imageId if publishId is set
-    const imageTag = splitFirst(explicitImage, ":")[1]
-    const parsedImage = containerHelpers.parseImageId(explicitImage)
-    const tag = imageTag || action.versionString()
-    imageId = containerHelpers.unparseImageId({ ...parsedImage, tag })
-  }
-
-  outputs.deploymentImageName = outputs["deployment-image-name"] = containerHelpers.getDeploymentImageName(
-    action.name,
-    imageId,
-    provider.config.deploymentRegistry
-  )
-  outputs.deploymentImageId = outputs["deployment-image-id"] = containerHelpers.getBuildDeploymentImageId(
-    action.name,
-    imageId,
-    action.moduleVersion(),
-    provider.config.deploymentRegistry
-  )
-
-  return outputs
+  return containerHelpers.getBuildActionOutputs(action, provider.config.deploymentRegistry)
 }
 
 function validateConfig<T extends ContainerModule>(params: ConfigureModuleParams<T>) {