-
Notifications
You must be signed in to change notification settings - Fork 23
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
NETOBSERV-1633 Konflux pipeline multiarch and downstream requirements #738
NETOBSERV-1633 Konflux pipeline multiarch and downstream requirements #738
Conversation
/hold PR not ready but need to run konflux to test |
5ddace0
to
96cffb1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #738 +/- ##
==========================================
- Coverage 65.47% 64.37% -1.10%
==========================================
Files 108 108
Lines 7026 8303 +1277
==========================================
+ Hits 4600 5345 +745
- Misses 2108 2639 +531
- Partials 318 319 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
c8b163f
to
53bbeeb
Compare
contrib/docker/Dockerfile.downstream
Outdated
FROM --platform=linux/$TARGETARCH registry.access.redhat.com/ubi9/ubi-minimal:9.4 | ||
|
||
COPY --from=builder /app/flowlogs-pipeline /app/ | ||
COPY --from=builder /app/confgenerator /app/ |
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.
iirc in cpaas midstream we did not include the confgenerator, it was only used in the upstream image
contrib/docker/Dockerfile.downstream
Outdated
COPY --from=builder /app/confgenerator /app/ | ||
|
||
# expose ports | ||
EXPOSE 2055 |
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'd also remove the expose in the downstream, that's a legacy thing for ipfix/netflows
@@ -1,22 +1,40 @@ | |||
#!/usr/bin/env bash | |||
ARG TARGETARCH | |||
FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:v1.22.5-202407301806.g4c8b32d.el9 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.
is that going to be automatically tracked and updated by konflux bot?
currently in midstream we're on go 1.22.7
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.
Yes konflux will automatically upgrade it.
LABEL com.redhat.component="network-observability-flowlogs-pipeline-container" | ||
LABEL name="network-observability-flowlogs-pipeline" | ||
LABEL io.k8s.display-name="Network Observability Flow-Logs Pipeline" | ||
LABEL io.k8s.description="Network Observability Flow-Logs Pipeline" | ||
LABEL summary="Network Observability Flow-Logs Pipeline" | ||
LABEL maintainer="[email protected]" | ||
LABEL io.openshift.tags="network-observability-flowlogs-pipeline" | ||
LABEL upstream-vcs-ref="${COMMIT}" | ||
LABEL upstream-vcs-type="git" |
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.
LABEL upstream-vcs-type="git" |
if you remove upstream-vcs-ref we should remove both
.tekton/pipeline-ref.yaml
Outdated
description: Path to a file with build arguments for buildah, see https://www.mankier.com/1/buildah-build#--build-arg-file | ||
name: build-args-file | ||
type: string | ||
- default: ["linux/x86_64", "linux/arm64"] |
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.
ibm power/z are not available?
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.
Fixed
82180ac
to
0780994
Compare
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.
LGTM
with 2 small suggestions to make it more similar to what we have in cpaas, but no blocker
0780994
to
2518c65
Compare
2518c65
to
6002d61
Compare
/lgtm |
6002d61
to
85b5f3a
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
…netobserv#738) * Konflux pipeline refactoring * Added downstream Dockerfile * Adding multi arch build to pipeline
Description
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.