Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tiltfile: fix ignores if context dir appears later #6467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Nov 11, 2024

Consider the following (admittedly somewhat unusual) Tiltfile:

trigger_mode(TRIGGER_MODE_MANUAL)

clone_cmd = "test -e tilt-avatars || git clone https://github.com/tilt-dev/tilt-avatars"
custom_build(
    "x-clone-repo",
    deps = [],
    skips_local_docker = True,
    command = clone_cmd,
)
docker_build(
    "x-build-image",
    context = "tilt-avatars",
    dockerfile_contents = "\n".join([
        "FROM x-clone-repo",
        "FROM scratch",
        "COPY ./ /opt/app/",
    ]),
    ignore = [
        "**/.git",
    ],
)
k8s_custom_deploy(
    name = "x-dummy-resource",
    apply_cmd = "true",
    delete_cmd = "true",
    deps = [],
    image_deps = ["x-build-image"],
)

This implements on-demand git cloning — the clone happens during the build phase of the resource, and thus other builds can proceed concurrently and this may speed up overall tilt up (and it does in the quite complex tilt setup that we have).

In current Tilt, however, ignore and only are completely ignored if the context directory doesn't exist when the Tiltfile is loaded. This is quite a strange and seemingly pointless limitation, as everything else seems to work fine.

The fix is trivial — simply remove the IsDir check from dockerignoresFromPathsAndContextFilters. The code that follows the check handles the absence of the dockerignore files just fine, so it seems the IsDir check is simply forgotten there from previous iterations.

Note that a similar issue exists in the repoIgnoresForPaths function as well. The Tiltfile to reproduce that problem is the same, just without the ignore = ["**/.git"] bit. I'm not entirely sure if that isGitRepoBase check serves any real purpose — would there be any harm if ".git" was ignored unconditionally (well, we'd need to check for the empty path like dockerignoresFromPathsAndContextFilters does)?

Related: #3048 (comment)

Consider the following (admittedly somewhat unusual) Tiltfile:

    trigger_mode(TRIGGER_MODE_MANUAL)

    clone_cmd = "test -e tilt-avatars || git clone https://github.com/tilt-dev/tilt-avatars"
    custom_build(
        "x-clone-repo",
        deps = [],
        skips_local_docker = True,
        command = clone_cmd,
    )
    docker_build(
        "x-build-image",
        context = "tilt-avatars",
        dockerfile_contents = "\n".join([
            "FROM x-clone-repo",
            "FROM scratch",
            "COPY ./ /opt/app/",
        ]),
        ignore = [
            "**/.git",
        ],
    )
    k8s_custom_deploy(
        name = "x-dummy-resource",
        apply_cmd = "true",
        delete_cmd = "true",
        deps = [],
        image_deps = ["x-build-image"],
    )

This implements on-demand git cloning — the clone happens during the
build phase of the resource, and thus other builds can proceed
concurrently and this may speed up overall tilt up (and it does in the
quite complex tilt setup that we have).

In current Tilt, however, `ignore` and `only` are completely ignored if
the context directory doesn't exist when the Tiltfile is loaded. This is
quite a strange and seemingly pointless limitation, as everything else
seems to work fine.

The fix is trivial — simply remove the IsDir check from
`dockerignoresFromPathsAndContextFilters`. The code that follows the
check handles the absence of the dockerignore files just fine, so it
seems the IsDir check is simply forgotten there from previous
iterations.

Note that a similar issue exists in the `repoIgnoresForPaths` function
as well. The Tiltfile to reproduce that problem is the same, just
without the `ignore = ["**/.git"]` bit. I'm not entirely sure if that
`isGitRepoBase` check serves any real purpose — would there be any harm
if ".git" was ignored unconditionally (well, we'd need to check for the
empty path like `dockerignoresFromPathsAndContextFilters` does)?

Related: tilt-dev#3048 (comment)
Signed-off-by: Tomas Janousek <[email protected]>
@liskin liskin force-pushed the on-demand-clone-fixes branch from 3a95022 to 33af3fd Compare November 11, 2024 13:48
@nicks
Copy link
Member

nicks commented Nov 19, 2024

hmmm...nice find!

i'm not sure this is the right fix. note that the integration tests are failing.

AFAIR the problem we ran into is that custom_build lets you depend on lots of individual files. and having lots of individual files in the ignore spec really blew up the matching engine and made everything slow.

what if we changed the logic to something like

generateIgnores := false
if isDir(path) {
  generateIgnores = true
} else if !exists(path) && isDockerBuild {
  generateIgnores = true
}

i think that would fix your use-case a bit more narrowly.

@nicks nicks self-requested a review November 19, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants