COS-3312: manifest-c9s: switch to building via container tools#167
Conversation
remove-from-packages doesn't work in the now build via container tools workflow so we'd need to convert any of these into a postprocess. I checked and there are no files in /usr/share/backgrounds at all any longer so let's just drop this entry in our manifests rather than convert it to a postprocess.
Support for RHCOS versioning was added in [1] so we cand drop this fork from our RHCOS repo now. [1] coreos/fedora-coreos-config@43b8d41
|
@dustymabe: This pull request references COS-3312 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.22.0" version, but no target version was set. DetailsIn 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request switches the c9s variant to be built using container tools, which is a significant and positive step. The changes involve moving configuration from YAML manifests to .conf files and adopting an upstream versioning script. The overall direction is good, but I've found a couple of issues that need attention. A change in common.yaml could unintentionally affect other build variants, and there appears to be a misconfiguration in the builder image for RHEL 10.1. My review includes suggestions to address these points.
I am having trouble creating individual review comments. Click here to see my feedback.
build-args-rhel-10.1.conf (10)
The BUILDER_IMG is set to use a path containing rhel9 for a rhel10.1 build. This is inconsistent with other variants (e.g., c10s uses a stream10 builder) and could lead to using an incorrect build environment. If this is not intentional, it should be corrected to use a rhel10 based builder image to ensure build consistency and correctness.
BUILDER_IMG=registry.stage.redhat.io/rhel10/rhel-bootc:10.1
common.yaml (363-365)
The remove-from-packages configuration is being removed from common.yaml. Since this file is included by all variants, this change will affect not just c9s, but also c10s, rhel-9.8, and rhel-10.1, which are not being switched to container builds in this PR. This will cause them to include /usr/share/backgrounds, which is likely unintended.
This configuration should be preserved for variants that are not built using containers. A good approach would be to make its inclusion conditional. For example, you could introduce a new variable like build_via_container in the manifests to control this behavior, ensuring that only c9s omits this configuration while other variants remain unaffected.
There was a problem hiding this comment.
Minor:
May be we need to add a new line here
Also there is a typo in the commit message
use versionary from fedora-coreos-config repo
Support for RHCOS versioning was added in [1] so we _**cand**_ drop
this fork from our RHCOS repo now.
There was a problem hiding this comment.
This is just how github shows that the file is now a symbolic link:
$ ls -l ./versionary
lrwxrwxrwx. 1 dustymabe dustymabe 31 Jan 30 16:36 ./versionary -> fedora-coreos-config/versionary
There was a problem hiding this comment.
Ah right, the symbolic linked files tend to have this .
| DESCRIPTION=RHEL CoreOS 9.8 | ||
| VERSION=9.8 | ||
| MUTATE_OS_RELEASE=9.8 | ||
| BUILDER_IMG=registry.stage.redhat.io/rhel9/rhel-bootc:9.8 |
There was a problem hiding this comment.
There is also another registry.stage here, may be this needs to be changed too?
There was a problem hiding this comment.
rhel 9.8 isn't released yet so we have to use the staging registry for that one
In this workflow more information is coming from the build-args. These changes take advantage of two upstream changes: - coreos/fedora-coreos-config@5c20a9e - coreos/fedora-coreos-config@e38ae10
This is part of https://issues.redhat.com/browse/COS-3312 and will switch the build of c9s over to the new way of building our base container images.
dustymabe
force-pushed
the
dusty-c9s-build-via-container-tools
branch
from
73d2f2e to
edf4c1a
Compare
|
/override ci/prow/scos-10-build-test-qemu See #163 |
|
@dustymabe: Overrode contexts on behalf of dustymabe: ci/prow/scos-10-build-test-qemu DetailsIn 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 kubernetes-sigs/prow repository. |
aaradhak
left a comment
aaradhak
left a comment
There was a problem hiding this comment.
Just the minor typo in the second commit message, other than that everything LGTM
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaradhak, dustymabe The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/scos-10-build-test-qemu See #163 |
|
@dustymabe: Overrode contexts on behalf of dustymabe: ci/prow/scos-10-build-test-qemu DetailsIn 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 kubernetes-sigs/prow repository. |
3 participants
Here are a few prep commits and a final commit for building c9s via container tools (i.e. podman build).
We are starting with c9s because there are no consumers of c9s right now (OKD uses c10s).