-
Notifications
You must be signed in to change notification settings - Fork 217
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
MGMT-17896: Add separate Dockerfile for assisted-service with an el8 base #6347
MGMT-17896: Add separate Dockerfile for assisted-service with an el8 base #6347
Conversation
This is required to ensure that users have an option to install FIPS clusters for versions where the installer is linked against el8 crypto libraries. The resulting image will be deployed by the operator when the user indicates that clusters requiring the el8 libraries will be installed in FIPS mode (versions < 4.16). Resolves https://issues.redhat.com/browse/MGMT-17896
@carbonin: This pull request references MGMT-17896 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6347 +/- ##
=======================================
Coverage 68.27% 68.28%
=======================================
Files 241 244 +3
Lines 35873 35958 +85
=======================================
+ Hits 24493 24553 +60
- Misses 9215 9239 +24
- Partials 2165 2166 +1 |
/retest |
Diff with https://github.com/openshift/assisted-service/blob/master/Dockerfile.assisted-service: 16c16
< FROM quay.io/centos/centos:stream9 as builder
---
> FROM quay.io/centos/centos:stream8 as builder
18c18
< RUN dnf install --enablerepo=crb -y gcc git nmstate-devel && dnf clean all
---
> RUN dnf install --enablerepo=powertools -y gcc git nmstate-devel && dnf clean all
42c42
< FROM quay.io/centos/centos:stream9
---
> FROM quay.io/centos/centos:stream8 Is there a way to have a common dnf install for both image ? If so, it might be worth it to add an ARG for the stream8/stream9 variation instead of adding a new file to maintain (like this https://docs.docker.com/reference/dockerfile/#understand-how-arg-and-from-interact) |
Using Ideally this will be temporary since we'll want to move toward the solution outlined in the body of #6290 in the long term. That said it may need to be a problem we solve for the installer runner images anyway, but hopefully those will be significantly simpler to build than assisted-service. |
/test e2e-agent-compact-ipv4 |
I'm not sure to see what you refer to when you talk about "confusion"
|
Sorry, I should have given a bit more information. That library (imagebuilder) is what is behind The "confusion" I'm referencing is that there was difference in behavior between how imagebuilder was handling these "heading args" and how docker handled it. All of our automated and published images should be using the imagebuilder code in some way, but I'm sure there are still people using docker locally so I wanted to avoid any chance of a very difficult to debug issue coming up due to that difference. That all said, I'll check to see if there's some straightforward way to parameterize this and use a single file. If not, I'd like to move forward with the different files for now given that this has a few other changes depending on it (all of which need to get in by the end of next week). We can then optimize later (or just wait until I remove them with the planned changes for the next release). On a separate note, and while you're here @pastequo ... How to I rerun the Konflux PR check that's failing here? Or at least how do I see what failed? When I click into the status I can see that something called a |
/retest should do the trick to re-run konflux pipeline @carbonin |
Instead of a separate Dockerfile for the el8 image an single build arg will determine the base image.
Moved to using a build arg. @adriengentil @pastequo can you take another look? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, carbonin 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 |
@carbonin: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
[ART PR BUILD NOTIFIER] This PR has been included in build ose-agent-installer-api-server-container-v4.17.0-202405240410.p0.g34706cf.assembly.stream.el9 for distgit ose-agent-installer-api-server. |
This is required to ensure that users have an option to install FIPS clusters for versions where the installer is linked against el8 crypto libraries.
The resulting image will be deployed by the operator when the user indicates that clusters requiring the el8 libraries will be installed in FIPS mode (versions < 4.16).
List all the issues related to this PR
Resolves https://issues.redhat.com/browse/MGMT-17896
What environments does this code impact?
How was this code tested?
Checklist
docs
, README, etc)Reviewers Checklist