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

docker: fix spurious double builds on Dockerfile change #5742

Merged
merged 2 commits into from
May 3, 2022

Conversation

milas
Copy link
Contributor

@milas milas commented Apr 27, 2022

Do not watch the Dockerfile from the image target, as this is
handled by the Tiltfile, which provides the full contents to the
target in-memory (i.e. the on-disk Dockerfile isn't actually used
when we execute the build).

This prevents race conditions from them both watching it, and
ensures that the flow always goes:
(Dockerfile change) -> Tiltfile execute -> image build

Fixes #5586.

Do not _watch_ the Dockerfile from the image target, as this is
handled by the Tiltfile, which provides the full contents to the
target in-memory (i.e. the on-disk Dockerfile isn't actually used
when we execute the build).

This prevents race conditions from them both watching it, and
ensures that the flow always goes:
    (Dockerfile change) -> Tiltfile execute -> image build

Fixes #5586.
@milas milas added the bug Something isn't working label Apr 27, 2022
@milas milas requested a review from nicks April 27, 2022 21:03
@milas milas self-assigned this Apr 27, 2022
@milas milas marked this pull request as ready for review April 28, 2022 12:53
Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! nice fix!

contextIgnores = append(contextIgnores, model.DockerignoresToIgnores(dockerignores)...)

for i := range contextIgnores {
fileWatchIgnores = append(fileWatchIgnores, *contextIgnores[i].DeepCopy())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi - i think the k8s api convention is to only do a copy when you know you're going to modify the object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is probably extra defensive but given the lifetime of these items and how much they get passed around (+ the fact that we've got two intersecting sets of them originating from the same objects), I figured an extra copy was worth saving potential future debugging headache down the road

internal/tiltfile/tiltfile_state.go Outdated Show resolved Hide resolved
@milas milas merged commit fc6b1c1 into master May 3, 2022
@milas milas deleted the milas/double-docker-build branch May 3, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change to a Dockerfile sporadically results in erroneous re-use of cached image
2 participants