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

Add downstream container files and update the konflux pipeline #90

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

msherif1234
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.61%. Comparing base (dfda635) to head (5e39cd9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #90   +/-   ##
=======================================
  Coverage   27.61%   27.61%           
=======================================
  Files          81       81           
  Lines        6999     6999           
=======================================
  Hits         1933     1933           
  Misses       4877     4877           
  Partials      189      189           
Flag Coverage Δ
unittests 27.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msherif1234 msherif1234 closed this Aug 5, 2024
@msherif1234 msherif1234 reopened this Aug 5, 2024
Copy link
Contributor

@frobware frobware left a comment

Choose a reason for hiding this comment

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

This looks like a copy of an existing containerfile, but I have some concerns regarding the use of go mod vendor during the build. This shouldn't be necessary and would lead to build inconsistencies.


# cache deps before building and copying source so that we don't need to re-download as much
# and so that source changes don't invalidate our downloaded layer
RUN go mod download
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the vendor directory already be up to date? The expectation is that go mod tidy and go mod vendor are run for each commit, ensuring consistency and avoiding the need to download dependencies locally or in CI.

Any commit should trigger a CI check for go mod tidy. If this check reports a difference, the commit is incomplete and should not be merged.

# Install crictl
RUN dnf -y install wget tar gzip
ARG VERSION="v1.28.0"
RUN wget --no-check-certificate https://github.com/kubernetes-sigs/cri-tools/releases/download/${VERSION}/crictl-${VERSION}-linux-${TARGETARCH}.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to disable certificate checking, especially for connections to github.com?

ARG TARGETARCH
ARG TARGETPLATFORM

RUN echo "TARGETOS=${TARGETOS} TARGETARCH=${TARGETARCH} BUILDPLATFORM=${BUILDPLATFORM} TARGETPLATFORM=${TARGETPLATFORM}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's double spaces between each env key/value pair that is being echoed.

# the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore,
# by leaving it empty we can ensure that the container and binary shipped on it will have the same platform.
WORKDIR /usr/src/bpfman-operator
RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -mod vendor -a -o bpfman-agent ./cmd/bpfman-agent/main.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the -a flag? We're building in a fresh container, so everything that needs building will get rebuilt regardless. Additionally, CI implicitly ensures a complete build.

@msherif1234 msherif1234 force-pushed the ds-dockerfiles branch 7 times, most recently from 93db2b3 to 6d92ddc Compare August 9, 2024 16:00
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Copy link
Contributor

@anfredette anfredette left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit bfe81d0 into bpfman:main Aug 9, 2024
20 checks passed
msherif1234 pushed a commit to msherif1234/bpfman-operator that referenced this pull request Nov 15, 2024
…/ocp-bpfman-operator-bundle

chore(deps): update ocp-bpfman-operator-bundle to 42b90d0
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.

3 participants