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

Bump ubi 9.4 #41

Merged
merged 4 commits into from
Aug 1, 2024
Merged

Bump ubi 9.4 #41

merged 4 commits into from
Aug 1, 2024

Conversation

jotak
Copy link
Member

@jotak jotak commented May 28, 2024

No description provided.

Dockerfile Outdated
@@ -26,10 +26,10 @@ RUN GOARCH=$TARGETARCH make compile
RUN mkdir -p output

# Create final image from ubi + built binary
FROM --platform=$TARGETPLATFORM registry.access.redhat.com/ubi9/ubi:9.3
FROM --platform=$TARGETPLATFORM registry.access.redhat.com/ubi9/ubi:9.4
Copy link
Member Author

Choose a reason for hiding this comment

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

guys, is there a reason here to use the "full" ubi and not the minimal like we do elsewhere ? Is it that we need more tooling in the image?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried building running e2e using ubi-minimal:9.4 it shows no issue I don't think we need the full image here you switched in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, commit added to use minimal
cc @jpinsonneau (in case not using minimal was intentional)

Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.08%. Comparing base (1c146cb) to head (dd7c69e).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #41   +/-   ##
=======================================
  Coverage   26.08%   26.08%           
=======================================
  Files           8        8           
  Lines        1081     1081           
=======================================
  Hits          282      282           
  Misses        778      778           
  Partials       21       21           
Flag Coverage Δ
unittests 26.08% <ø> (ø)

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

@jpinsonneau
Copy link
Contributor

/retest

@jotak
Copy link
Member Author

jotak commented Jul 26, 2024

@jpinsonneau , looking at the error, it seems like in the e2e test "TestFlowCapture", the output/flow directory does not exist for some reason, when using a minimal base image. Any idea why?

konflux bot is also upgrading the ubi to 9.4: #70 - so if we think this is good enough and we don't need the minimal image, I can close this PR. But I'd be curious to understand what makes it fail with minimal

@jpinsonneau
Copy link
Contributor

@jpinsonneau , looking at the error, it seems like in the e2e test "TestFlowCapture", the output/flow directory does not exist for some reason, when using a minimal base image. Any idea why?

konflux bot is also upgrading the ubi to 9.4: #70 - so if we think this is good enough and we don't need the minimal image, I can close this PR. But I'd be curious to understand what makes it fail with minimal

UBI 9.3 contains package tar-1.34-6.el9_1.x86_64 GNU file archiving program
which is not present in minimal container image.

As discussed in this slack thread, you need to have tar installed on the container side to properly run oc cp.

Did you try the rsync alternative ?

@jotak
Copy link
Member Author

jotak commented Jul 29, 2024

let's see if adding tar fixes it then (I prefer to stick with the current behaviour as much as possible, and adding tar was something we do downstream also)

@jotak
Copy link
Member Author

jotak commented Jul 29, 2024

/retest

@jotak
Copy link
Member Author

jotak commented Jul 29, 2024

@jpinsonneau e2e pass now \o/

Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

Code looks good, thanks @jotak !

@jotak
Copy link
Member Author

jotak commented Jul 31, 2024

@jpinsonneau I think this change will be necessary for the prow/image test to pass

@jotak
Copy link
Member Author

jotak commented Jul 31, 2024

/test images

@jotak
Copy link
Member Author

jotak commented Aug 1, 2024

/approve

Copy link

openshift-ci bot commented Aug 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Aug 1, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d4d4c9e into netobserv:main Aug 1, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants