-
Notifications
You must be signed in to change notification settings - Fork 35
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
Align downstream dockerfile with the upstream one #557
Align downstream dockerfile with the upstream one #557
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-1.8 #557 +/- ##
===============================================
+ Coverage 27.48% 27.61% +0.12%
===============================================
Files 46 46
Lines 4940 4940
===============================================
+ Hits 1358 1364 +6
+ Misses 3478 3473 -5
+ Partials 104 103 -1
Flags with carried forward coverage won't be shown. Click here to find out more. |
Dockerfile.downstream
Outdated
@@ -1,27 +1,27 @@ | |||
ARG TARGETARCH | |||
|
|||
# Build the manager binary | |||
FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:v1.23 as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't u need --platform=linux/$TARGETARCH here as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it, thanks!
@@ -1,27 +1,27 @@ | |||
ARG TARGETARCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you don't also need to add ARG COMMIT
here, and also, move the one line 8 to the next layer after line 24
I'm comparing this file with https://github.com/netobserv/flowlogs-pipeline/blob/main/contrib/docker/Dockerfile.downstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added it, thanks!
42d1184
to
4074757
Compare
@@ -1,27 +1,30 @@ | |||
ARG TARGETARCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARG TARGETARCH | |
ARG TARGETARCH | |
ARG COMMIT |
Shouldn't it be on the top ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks
4074757
to
33eec8a
Compare
Dockerfile.downstream
Outdated
ARG VERSION="unknown" | ||
ARG COMMIT | ||
ARG LDFLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm who set that argument I checked flp and operator they don't seem to use it ? does konflux pipeline set it ? I don't think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updstream Dockerfile for the ebpf agent is using one:
netobserv-ebpf-agent/Dockerfile
Line 7 in cee90d9
ARG LDFLAGS |
This is to align with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but its set in the Makefile I am not sure if konflux pipeline will set tbh ? pls check konflux build log to see if its set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if its not set then we need to set like in https://github.com/netobserv/netobserv-ebpf-agent/blob/main/Makefile#L29
Dockerfile.downstream
Outdated
|
||
# Build | ||
RUN make compile | ||
RUN GOARCH=$TARGETARCH go build -ldflags "$LDFLAGS" -mod vendor -a -o bin/netobserv-ebpf-agent cmd/netobserv-ebpf-agent.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind to fix FIPS at the same time :-)
RUN GOARCH=$TARGETARCH go build -ldflags "$LDFLAGS" -mod vendor -a -o bin/netobserv-ebpf-agent cmd/netobserv-ebpf-agent.go | |
ENV GOEXPERIMENT strictfipsruntime | |
RUN GOARCH=$TARGETARCH go build -tags strictfipsruntime -ldflags "$LDFLAGS" -mod vendor -a -o bin/netobserv-ebpf-agent cmd/netobserv-ebpf-agent.go |
8c98a1e
to
a8a940a
Compare
Co-authored-by: Julien Pinsonneau <[email protected]>
a8a940a
to
c798bf6
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ba3e933
into
netobserv:release-1.8
* Align downstream dockerfile with the upstream one * Update Dockerfile.downstream Co-authored-by: Julien Pinsonneau <[email protected]> --------- Co-authored-by: Julien Pinsonneau <[email protected]>
* Align downstream dockerfile with the upstream one (#557) * Align downstream dockerfile with the upstream one * Update Dockerfile.downstream Co-authored-by: Julien Pinsonneau <[email protected]> --------- Co-authored-by: Julien Pinsonneau <[email protected]> * Fix arg usage for downstream builds (#558) --------- Co-authored-by: Julien Pinsonneau <[email protected]>
Description
This fix the multiarch build with konflux.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.