rework manifests/overlays#4227
Conversation
Make the comment at the top more accurate and add some debug logs for the arguemnts in the args file that are passed in so we can see what's happening under the hood during a build.
Previously ostree-layers entries strings had to start with '/overlay' and it was hardcoded to look into the overlay.d/ directory for them. Rather than this let's just have the strings in the list be the actual directory the override resides in. Also here we rename a bunch of variables in build-rootfs to be add_dirs instead of overlays, because the argument to bootc-base-imagectl is --add-dir.
This should help it show up better in the logs.
This will make it so they don't have to be redefined in rhel-coreos-config.
This is not just the default, it's the only option now. Drop this setting.
We are now on F44 and this postprocess is dead code.
It didn't really make sense to have two files for this. Should be no funcitonal diff from this change.
This will allow us to define them here so we don't have to redefine them downstream. As long as shared-el.yaml gets included downstream the rest of these will get included.
|
I still need to do a local run with and without this change to make sure there are no significant differences. |
![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 refactors the Fedora CoreOS manifest structure by removing the base manifest and consolidating packages, postprocess scripts, and ostree-layers directly into fedora-coreos.yaml and shared-el.yaml. It also updates build-rootfs to use gather_add_dirs instead of gather_overlays, removing the strict requirement for layers to start with overlay/, and adds debugging output for the bootc-base-imagectl arguments. Feedback on these changes suggests improving the robustness of the manifest parsing in build-rootfs by using manifest.get('ostree-layers') or [] to avoid a potential TypeError if the key is explicitly set to null in the YAML configuration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Approach sounds good. This should simplify the manifests in rhel-coreos-config if I understand correctly. Can you make a companion PR for that? I have not checked if the move was exhaustive so we'll have to make sure that there are no or next to no change from this change. |
yep. would look something like coreos/rhel-coreos-config#276
yeah. I mentioned in #4227 (comment) basically will need to |
There was a problem hiding this comment.
I find overlays to be more intuitive here given that the actual directories are prefixed overlay and how the contents are merged into the tree is like overlays are.
I understand it's not technically correct but dirs is generic and does not really give much information, while overlay does.
Maybe the bootc option should be merge-tree to be more explicit :)
/end of bikeshedding
There was a problem hiding this comment.
yeah. maybe the bootc-base-imagectl --add-dir should be changed to --overlay-dir ? If you want to PR that change we can then update it here?
I ran the diffs for this repo and everything looked good. Marking as ready for review now. Will also run the diffs for some |
These look good too |
yasminvalim
left a comment
yasminvalim
left a comment
There was a problem hiding this comment.
Left a question in the review, but otherwise looks good to me! 👍
In coreos/fedora-coreos-config#4227 we reworked things upstream to define more of what is defined in rhel-coreos-config in the shared-el.yaml manifest. This means we can manage the definition of the "fedora-coreos pieces" of the rhel-coreos-config directly upstream and changes to that definition are self contained and brought in via submodule bumps. This commit adjusts the defnitions here to drop the pieces that are now redundant because they are defined upstream in shared-el.yaml.
4 participants
Some rework of the manfifests and overlays here. See individual commit messages.
This should enable us to define more downstream shared definitions here in this repo without having to symlink or redefine them downstream.