fix: make IMAGE_TAG available in buildArgs when used in docker FROM

[alphanota]

Note: This PR has the cherry-picked changes from @abe-winter 's original PR #9450.
Related:

• related fix for kaniko Make IMAGE_REPO, IMAGE_NAME, IMAGE_TAG available in build.artifacts[i].docker.buildArgs #4295
• related fix fix: Fill IMAGE_TAG,etc on Docker builds #7788

Description
Apologies if I am simply misunderstanding something and this can be fixed w/out a code change. I am certainly not fully wrapping my head around this codebase.

This PR adds the ability to use IMAGE_TAG (IMAGE_REPO etc) in the docker buildArgs section, specifically when the build arg is then consumed in the Dockerfile FROM. On current stable and main, this case fails in the depList / SingleArtifactDependencies phase, with logs like:

Checking cache...

• base: Found Remotely
• child: Error checking cache.

getting hash for artifact "child": getting dependencies for "child": parsing ONBUILD instructions: retrieving image "gcr.io/headsdown/base:": parsing reference "gcr.io/headsdown/base:": could not parse reference: gcr.io/headsdown/base:

Note that when used with dependencies (as below), this seems to only work reliably if I run skaffold build && skaffold run. skaffold run by itself will sometimes fail with 'image not found remotely'.

Repro

Stripped-down skaffold.yaml:

build:
  local:
    useBuildkit: true
  artifacts:
    - image: base
      docker:
        dockerfile: Dockerfile
    - image: child
      requires:
      - image: base
      docker:
        dockerfile: child/Dockerfile
        buildArgs:
          IMAGE_TAG: '{{.IMAGE_TAG}}'

Top of child/Dockerfile:

# syntax=docker/dockerfile:1
ARG IMAGE_TAG
FROM gcr.io/PROJECTNAME/base:$IMAGE_TAG

tldr the second artifact depends on the first and then uses it as a base image

User facing changes
I think the docs already imply this is possible

Follow-up Work
No idea if I'm conforming to house style -- if accepted, guessing someone on the team will need to clean up or at least assign me cleanup. I would normally have submitted a bug rather than a PR for something like this, but I didn't understand why my template param was missing until I'd already solved the problem.

If this is something useful, happy to add tests etc.

@alphanota : We might want to add integration tests to check that the build args are resolved correctly.

[code-review-assist]

Hi team,

I'm currently reviewing this pull request and will have a full review in a few minutes. In the meantime, here's a summary to help you and other reviewers quickly get up to speed:

This pull request addresses an issue where the IMAGE_TAG variable wasn't correctly available in buildArgs when used within a Docker FROM instruction. The problem manifested as errors during the dependency resolution phase of the build process, resulting in messages like could not parse reference: gcr.io/headsdown/base:<no value>.

Intent: The primary goal is to enable the use of IMAGE_TAG (and related variables like IMAGE_REPO) within buildArgs when those args are used to define the base image in a Dockerfile. This allows for more dynamic and flexible image building.

Changes:

The changes involve modifying several functions related to dependency listing and cache handling within the Skaffold build process. Specifically:

• The DependencyLister function signature in pkg/skaffold/build/cache/cache.go was updated to include a tags map, allowing for the propagation of image tags.
• The hash function in pkg/skaffold/build/cache/hash.go was modified to incorporate this tags map, ensuring that the cache correctly accounts for the image tag values.
• Several other functions related to cache lookup and dependency resolution were updated to handle the tags map consistently, including pkg/skaffold/build/cache/lookup.go, pkg/skaffold/build/gcb/cloud_build.go, pkg/skaffold/diagnose/diagnose.go, and pkg/skaffold/runner/new.go.
• A new helper function quickMakeEnvTags was added to pkg/skaffold/graph/dependencies.go to parse and create the tags map from an image reference.
• The SingleArtifactDependencies function in pkg/skaffold/graph/dependencies.go was modified to accept and use the tags map.
• The EvalBuildArgs function in pkg/skaffold/graph/dependencies.go was updated to EvalBuildArgsWithEnv, which takes the environment tags into account.

Activity: The pull request author, alphanota, provided a detailed description of the problem, including a reproduction scenario and a proposed solution. They also acknowledged potential style issues and expressed willingness to make further improvements.

I'll provide a more detailed review shortly, but this summary should give you a good overview of the changes and their purpose.

And now, a little haiku to brighten your day:

Tags flow, code compiles,
Building images, smooth and fast,
Skaffold's power shines.

[code-review-assist]

This PR addresses a critical issue where IMAGE_TAG (and related variables) were not available in buildArgs when used within the FROM instruction of a Dockerfile. The solution correctly propagates the tag information to the dependency resolution stage. However, the introduction of quickMakeEnvTags introduces a potential point of failure and adds complexity. I recommend exploring alternative approaches that leverage existing Skaffold mechanisms for resolving tags.

[plumpy]

Ideally I'd love to see an integration test that uses these new args. But at the very least, there should be some unit tests.

[plumpy]

hash_test doesn't test this new behavior at all

[plumpy]

I would probably call this stubArtifactResolver. Googling "stub vs mock" leads to 100s of pages with a variety of explanations, but here's a somewhat reasonable one:

A stub provides predetermined responses to calls made during a test. ... These responses are usually based on hard coded values or objects pre-programmed to return specific outputs, making them predictable.

[plumpy]

there are no unit tests for this new behavior