bake: Support preservation of .git directory for remote context

[marxarelli]

(updated implementation)

When a remote Git URL is used with buildx bake, .git directories are
always omitted (see bake.ReadRemoteFiles).

Targets that rely on the default bake context should still be able to
change this behavior by passing the BUILDKIT_CONTEXT_KEEP_GIT_DIR=1
build argument. Check for this argument and skip the use of the bake
context state if the value is set to true.

---

(original implementation)

When a remote URL is used with buildx bake, the resulting remote context is reused as the default main context of bake targets. Git remote contexts are handled using dockerui.DetectGitContext which accepts a keepGitDir argument and uses that for the llb.KeepGitDir Git source option.

Prior to this change, the value for keepGitDir was hardcoded as false, resulting in a main context for targets that never retains the .git directory from the cloned repo even where
BUILDKIT_CONTEXT_KEEP_GIT_DIR is defined as a build argument in a bake target definition.

Support preservation of the .git directory in main target contexts when using buildx bake with remote Git URLs via a new environment variable BUILDX_BAKE_KEEP_GIT_DIR. This mirrors the behavior of BUILDKIT_CONTEXT_KEEP_GIT_DIR for build targets and is keeping with the convention of other environment variables that effect remote bake contexts such as BUILD_BAKE_GIT_SSH.

Note that for a target to retain the .git directory, both the BUILDX_BAKE_KEEP_GIT_DIR environment variable and BUILDKIT_CONTEXT_KEEP_GIT_DIR build argument must be set to a true value, the latter being specific to each target that inherits the remote Git context.

[marxarelli]

@tonistiigi for your review.

Note that this still requires folks to know about the additional environment variable. An alternative approach that might be less surprising to users but less efficient by default, would be to always keep git directories on the remote bake context. In that case, the BUILDKIT_CONTEXT_KEEP_GIT_DIR build argument for individual targets would work as (I originally) expected without having to know about the additional environment variable.

I'm happy to refactor if you think that's a better approach.

[tonistiigi]

Isn't this supposed to come from the bake definition itself. ReadRemoteFiles itself should never need git dir, but after it has read the bake definition and it has been parsed it could detect that the target set BUILDKIT_CONTEXT_KEEP_GIT_DIR build arg and in that case switch the inp.State over to the one that has git dir enabled?

[marxarelli]

Isn't this supposed to come from the bake definition itself. ReadRemoteFiles itself should never need git dir, but after it has read the bake definition and it has been parsed it could detect that the target set BUILDKIT_CONTEXT_KEEP_GIT_DIR build arg and in that case switch the inp.State over to the one that has git dir enabled?

I like this approach in that it would obviate the need for an additional environment variable. I also like that it comes from the bake definition, like you mentioned.

The only downside I see is that we can't reuse the bake input state and thus there will be duplicate clone operations (two distinct source identifiers, one where KeepGitDir is always false, and one where it might be true). However, this inefficiency also arises with the change I made here, where BUILDX_BAKE_KEEP_GIT_DIR=1 is set but the targets don't specify BUILDKIT_CONTEXT_KEEP_GIT_DIR=1.

Maybe we do both?

For correctness: detect targets that set BUILDKIT_CONTEXT_KEEP_GIT_DIR=1 and use their context state instead of the bake input state.

For optimization: allow users to set BUILDX_BAKE_KEEP_GIT_DIR=1 and avoid duplicate clones because although the input state will not be reused, at least the cache keys should match.

[tonistiigi]

The only downside I see is that we can't reuse the bake input state and thus there will be duplicate clone operations (two distinct source identifiers, one where KeepGitDir is always false,

This shouldn't be a big problem. llb.Git implementation uses a shared repository for all reference lookups, so you would still only get one remote clone that is then checked out two different ways. I have a draft of a sparse checkout implementation as well. If that one ever lands, we would probably always use that for ReadRemoteFiles.

[marxarelli]

This shouldn't be a big problem. llb.Git implementation uses a shared repository for all reference lookups, so you would still only get one remote clone

Ah! In that case, I will attempt to implement the build arg detection.

[marxarelli]

@tonistiigi I've amended the commit and updated the PR description to reflect the new approach.

[marxarelli]

@tonistiigi I saw this was (seemingly) missing while implementing my change. Without it, the context is simply reassigned below. Let me know if my assumption isn't correct and I'll remove it.

[crazy-max]

LGTM thanks!

[tonistiigi]

Not sure I get this. Shouldn't this only run in the t.ContextPath == "." case above?

[tonistiigi]

Iiuc this is building from a local directory (that contains .git), not the git address as first parameter.

[crazy-max]

Seems to be the git address as first arg (see next line). withDir is used to set working dir when running command.

found issue