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.

Sign up for GitHub

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

Add a dockerignore #1962

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

Add a dockerignore #1962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,15 @@ FROM --platform=$BUILDPLATFORM base AS builder
COPY . .

ARG TARGETARCH
ARG CGO_ENABLED=0
ARG BPF2GO_CFLAGS="-I/usr/src/go.opentelemetry.io/auto/internal/include/libbpf -I/usr/src/go.opentelemetry.io/auto/internal/include"
RUN --mount=type=cache,target=/root/.cache/go-build \
--mount=type=cache,target=/go/pkg \
GOARCH=$TARGETARCH make build
GOARCH=$TARGETARCH \
CGO_ENABLED=$CGO_ENABLED \
BPF2GO_CFLAGS=$BPF2GO_CFLAGS \
go generate ./... \
&& go build -o otel-go-instrumentation ./cli/...
Comment on lines +24 to +28
Copy link
Contributor

@RonFed RonFed Mar 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think I prefer calling the makefile here,
The main reason being we don't need to duplicate the env vars setup and having a single source of truth for building.
If we want to avoid go-mod tidy and linting we can probably add a target without those dependencies?
Although I don't think it adds a lot of latency to the build (the go generate probably takes most of the time anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer we avoid the circular dependency of make calling this and then this calling make.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree with @RonFed, calling make build reduces the need to make sure changes are duplicated

Copy link
Contributor Author

@MrAlias MrAlias Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Adding a one-off target to the Makefile just to run these commands does not seem appropriate to me.

It is a one-off command that is only ever meant to be run by the Dockerfile. Why not just add the command to the Dockerfile?

Additionally, it is adding the command to a development tooling file, but it is not meant to be run by developers. What happens when we get an issue asking why the new make target fails because their repo was not correctly linted before-hand? That seems like a valid complaint. They are running developer tooling, its just they don't have the domain history of the project to know that this "one special target" isn't meant for them to run.

Running the make target also means that we will need to copy over the Makefile. This, again, will mean that unrelated changes to the repo (the Makefile) will invalidate the docker cache. This happens a lot in developement.

I don't see adding a new include directory to BPF2GO_CFLAGS in both the Makefile and the Dockerfile as a burden given how little this happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Calling make build from the dockerfile is common for the reasons Ron and I mentioned. But, this isn't a huge deal

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, it is adding the command to a development tooling file, but it is not meant to be run by developers. What happens when we get an issue asking why the new make target fails because their repo was not correctly linted before-hand? That seems like a valid complaint. They are running developer tooling, its just they don't have the domain history of the project to know that this "one special target" isn't meant for them to run.

This can be addressed by improving the Makefile.
For example when the build target is triggered, it can check if it is running on linux, otherwise return an informative error.

Copy link
Contributor Author

@MrAlias MrAlias Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@RonFed I don't follow. I develop on a Linux system, why would I want it to succeed for me but not for a Mac dev?

Also, that wouldn't address the docker cache invalidation I am trying to address.


FROM gcr.io/distroless/base-debian12@sha256:74ddbf52d93fafbdd21b399271b0b4aac1babf8fa98cab59e5692e01169a1348
COPY --from=builder /usr/src/go.opentelemetry.io/auto/otel-go-instrumentation /
Expand Down
13 changes: 13 additions & 0 deletions Dockerfile.dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
*

# must-have toplevel files
!/go.sum
!/go.mod
!*.go
!/LICENSE

# directories
!/cli
!/sdk
!/internal/include
!/internal/pkg
Loading