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

Ignore Dockerfile/Compose files by default #51

Merged
merged 2 commits into from
Feb 18, 2025
Merged

Conversation

lionello
Copy link
Member

@lionello lionello commented Feb 18, 2025

The Dockerfile and compose.*.yaml is not needed inside the build, so we should ignore it by default. Ideally the runtime images only have runtime files in them, so no build-only artifacts. If these do get included, there are a couple of unwanted side-effects:

  • Changing compose.yaml for one service would cause all services to get rebuilt if the compose.yaml is in their build contexts, even though only one service config got changed.
  • Sometimes (lazy) devs add secrets into the compose.yaml and these could end up being copied into the runtime container image because of a COPY . . statement.
  • A change to a Dockerfile doesn't always result in a new image, because the resulting image might be the same, but if the Dockerfile ends up being copied into the image, that it will by definition always change the image and cause a service replacement.

Should we include the .dockerignore in the build context? This way, Kaniko will use it when it evaluates COPY . . etc. But a bit tricky to be including files that weren't there to begin with. We'd have to ignore .dockerignore itself, for one.

Copy link
Member

@jordanstephens jordanstephens left a comment

Choose a reason for hiding this comment

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

Won't this exclude Dockerfile by default when deploying any service (assuming none of them have their own .dockerignore) and therefore break all deployments?

@lionello
Copy link
Member Author

Won't this exclude Dockerfile by default when deploying any service (assuming none of them have their own .dockerignore) and therefore break all deployments?

@jordanstephens glad you asked! No:

pulumi-defang/upload.ts

Lines 93 to 94 in 11e790f

if (!foundDockerfile && normalized === dockerfile) {
return (foundDockerfile = true); // we need the Dockerfile, even if it's in the .dockerignore file

@jordanstephens
Copy link
Member

jordanstephens commented Feb 18, 2025

Won't this exclude Dockerfile by default when deploying any service (assuming none of them have their own .dockerignore) and therefore break all deployments?

@jordanstephens glad you asked! No:

pulumi-defang/upload.ts

Lines 93 to 94 in 11e790f

if (!foundDockerfile && normalized === dockerfile) {
return (foundDockerfile = true); // we need the Dockerfile, even if it's in the .dockerignore file

I'm confused then. I think I'm missing some context to understand these statements:

The Dockerfile is not needed inside the build, so we should ignore it by default.

And

we need the Dockerfile, even if it's in the .dockerignore file

What do we gain by making these changes? It's not clear from the description or from our discussion so far.

@lionello
Copy link
Member Author

lionello commented Feb 18, 2025

@jordanstephens You're right there's still something missing, which is what I meant with "Should we include the .dockerignore in the build context?"

The important "fix" is that we ignore the Dockerfile (and compose.*.yaml etc.) when the Dockerfile itself is being evaluated and has a COPY . . command, ie. "copy all files from build context to current folder". There's no reason why we need the Dockerfile in the final image file system. So we want the builder to ignore Dockerfile etc.

But when we (Defang) create the tarball for the builder, we always need to include the Dockerfile, since the builder needs it. That's where that extra logic comes in "we need the Dockerfile, even if it's in the .dockerignore file".

What's missing is that when we use the default baked-in .dockerignore, we don't include this file in the tarball, so when it gets to the builder, there's no .dockerignore and it will still include the Dockerfile. I don't know what's a good solution to this. I'm hesitant to sneakily add files to the tarball. But in the very least we have a good proto .dockerignore, which eventually we could let users bootstrap their project with it.

@lionello lionello changed the title Ignore Dockerfile by default Ignore Dockerfile/Compose files by default Feb 18, 2025
@lionello
Copy link
Member Author

In the Kaniko case, there's a flag that we can use to ignore specific paths https://github.com/GoogleContainerTools/kaniko?tab=readme-ov-file#flag---ignore-path

@jordanstephens
Copy link
Member

I guess I'm not sure why it matters to pull the user's Dockerfile and compose files from the tarball. I think I hear you that it's probably not necessary for the running application, but we don't actually know that. Hell, the application could read the Dockerfile or compose file at startup for some reason—we don't know. What are we fixing by removing it?

@lionello
Copy link
Member Author

What are we fixing by removing it?

@jordanstephens unnecessary rebuilds by leaving Dockerfile/Compose out of the built image. Note that this is the default .dockerignore for when the project has none.

@jordanstephens
Copy link
Member

What are we fixing by removing it?

@jordanstephens unnecessary rebuilds by leaving Dockerfile/Compose out of the built image. Note that this is the default .dockerignore for when the project has none.

Shouldn't a changed Dockerfile trigger a rebuild?

@jordanstephens
Copy link
Member

After discussing with @lionello , he suggested following up with DefangLabs/defang#1015 so that the default .dockerignore is transparent and mutable by the user.

@lionello
Copy link
Member Author

@jordanstephens and I agreed this is a temp workaround and that the better option is to have a way for people to instantiate a .dockerignore so it's transparent and they can edit it.

@lionello lionello merged commit 69380a6 into main Feb 18, 2025
2 checks passed
@lionello lionello deleted the lio/ignore-dockerfile branch February 18, 2025 20:41
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