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

🌱 update dockerfiles to just copy the code #4525

Conversation

kannon92
Copy link

Fixes #4524

As I have worked on a few kubebuilder based projects, I always find over time that I switch to using a similar behavior for the template.

It causes quite a bit of confusion as it is usually not a common practice when you add folders that you must remember to add them to the Dockerfile also.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kannon92
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Kavinjsir
Copy link
Contributor

Hey @kannon92 splitting out dependency installation steps in your Dockerfile can help Docker build cache more efficiently and usually speeds up the build process. You might also want to check out the official Docker CLI for generating a Go-specific Dockerfile—it shows exactly why copying go.mod and go.sum in separate steps is a best practice.

# Download dependencies as a separate step to take advantage of Docker's caching.
# Leverage a cache mount to /go/pkg/mod/ to speed up subsequent builds.
# Leverage bind mounts to go.sum and go.mod to avoid having to copy them into
# the container.
RUN --mount=type=cache,target=/go/pkg/mod/ \
    --mount=type=bind,source=go.mod,target=go.mod \
    go mod download -x

# This is the architecture you're building for, which is passed in by the builder.
# Placing it here allows the previous steps to be cached across architectures.
ARG TARGETARCH

# Build the application.
# Leverage a cache mount to /go/pkg/mod/ to speed up subsequent builds.
# Leverage a bind mount to the current directory to avoid having to copy the
# source code into the container.
RUN --mount=type=cache,target=/go/pkg/mod/ \
    --mount=type=bind,target=. \
    CGO_ENABLED=0 GOARCH=$TARGETARCH go build -o /bin/server ./api

COPY cmd/main.go cmd/main.go
COPY api/ api/
COPY internal/ internal/
COPY . .
Copy link
Member

@camilamacedo86 camilamacedo86 Jan 26, 2025

Choose a reason for hiding this comment

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

Thank you for your contribution. 🎉

The proposed change replaces the current approach, where we only add specific paths (cmd/, api/, internal/, etc.), with COPY . ., which adds everything from the root of the project. Am I correct?

This means that unnecessary files and directories like hack/, test/, README, and other root-level files will now be included in the manager image, even if they are not required.

While this might seem convenient, however, it shows like goes against best practices by unnecessarily bloating the image. That is why we promote to explicitly copy only the paths we need to ensure a clean and optimized image, avoiding the inclusion of unrelated files. See: https://docs.docker.com/build/building/best-practices/#dont-install-unnecessary-packages

Does that make sense?

/hold

Copy link
Member

Choose a reason for hiding this comment

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

Could we agree to close this one?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2025
@camilamacedo86
Copy link
Member

Hi @kannon92

Thank you a lot for your contribution 🥇 and for looking to make kubebuilder

But as@Kavinjsir pointed out: #4525 (comment)
Also, see my comment: #4525 (comment)

What you suggested does not seem to align with Docker's best practices: https://docs.docker.com/build/building/best-practices/#dont-install-unnecessary-packages

Therefore, I do not think that we should not move forward here.
Does that make sense ?

So, I hope that you do not mind, but I am closing this one and the PR as deferred

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerfile templates always cause confusion
4 participants