Switch Satellite builds to containerized - #4995
Conversation
github-actions
Bot
added
Needs tech review
|
This is a big step. 🎉 |
Lennonka
left a comment
Lennonka
left a comment
There was a problem hiding this comment.
The changeset looks sane to me.
|
Similar to #4905, I extended the find_unused_modules script with exceptions. These are needed because quite a few modules really are unused now -- they are satellite-only but not yet containerized, so they are not included anywhere (yet). |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Please consider all the inline comments as thoughts when finally pulling the trigger.
I'm not strictly opposed to this, but there is a risk to having unintended consequences when doing cherry picks. Have you considered this risk?
|
I've implemented the feedback, and along with the list of exceptions in the |
|
We had a separate conversation about the cherry-picking concerns elsewhere, so let me summarize it here for maximum transparency: There are some concerns due to our cherry-picking workflows -- removing the non-containerized build in preview might create a blind spot: If I update something on master and only view the containerized preview, I might accidentally break things on the non-containerized preview after the update is cherry-picked to 3.19. However, the other build targets still include non-containerized previews, which would mitigate the risk. Option 1: If we want to feel extra safe about preventing that risk, we could mandate separate cherry-picking PRs for branches 3.19 and below (the workflow would be 1. open a PR against master + get it reviewed, 2. open a new PR against 3.19 with the same changes + get it reviewed). Option 2: If we don't feel like we need this extra safeguard and the non-satellite non-containerized previews are enough, we should be able to continue cherry-picking as usual. Without mandating a separate PR for 3.19. Please @ekohl @jafiala @Lennonka feel free to review if I captured all arguments and counter-arguments correctly. |
|
As the author of the PR, I'm leaning towards Option 2 :) provided that all maintainers are mindful of the difference between containerized and non-containerized. Which, I think, by now we all are. |
|
Option 2. |
aneta-petrova
removed
the
Needs tech review
|
Option 2 as well. |
aneta-petrova
added
style review done
| :project-client-RHEL8-url: {RepoRHEL8ServerSatelliteToolsProjectVersion} | ||
|
|
||
| // Containerized deployment overrides | ||
| :containerized: |
There was a problem hiding this comment.
I suggested to set the containerized attribute in attributes-containerized.adoc so it's set for all containerized builds. Then you can also drop it from the ifeval::["{build}" == "containerized-katello"] and ifeval::["{build}" == "containerized-orcharhino"] sections in guides/common/attributes.adoc.
There was a problem hiding this comment.
This PR is about making changes to Satellite build targets and I'd rather not touch the other ones. In #4995 (comment), I explained I'm making the change only for Satellite.
Since the satellite build is now containerized-only, modules and assemblies that are excluded for containerized builds (wrapped in ifndef::containerized[]) are no longer included in any build target. Add exceptions for these 82 files to prevent find_unused_modules from reporting them as errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
75783b5 to
4f8b7c5
Compare
|
A rebase was needed to pull in a new attribute added in https://github.com/theforeman/foreman-documentation/pull/4996/changes |
5 participants
What changes are you introducing?
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
We shouldn't need two separate Satellite builds anymore
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Assisted-by: Claude
Contributor checklists
Please cherry-pick my commits into: