Satellite flavor - #726
Conversation
| active_flavor = config.user_parameters.flavor | ||
| active_flavor_dir = FLAVOR_TESTS_DIR / active_flavor | ||
| # if there is an override, use that, otherwise use the flavor verbatim | ||
| active_flavor_dir = FLAVOR_TESTS_DIR / FLAVOR_TESTS_DIR_OVERRIDES.get(active_flavor, active_flavor) |
There was a problem hiding this comment.
this means we can't have a tests/flavor/satellite that adds satellite-specific tests. I don't think that's bad (we can place them somewhere else, if we ever have any), but wanted to call it out here.
There was a problem hiding this comment.
In #722 I wanted to go all the way to feature-based testing anyway and reduce the flavor-based testing. I see here you already took the approach of exposing internal features (just like I did) so perhaps we can just get rid of the flavor based tests anyway?
tests/flavor/katello/postgresql_test.py already has logic that is more automated in tests/backup_test.py while tests/flavor/katello/images_test.py could also be easily written to look at enabled features.
The foreman-proxy-content are a bit more special, but that's not a concern for this particular PR.
94a065c to
f4c5afd
Compare
| active_flavor = config.user_parameters.flavor | ||
| active_flavor_dir = FLAVOR_TESTS_DIR / active_flavor | ||
| # if there is an override, use that, otherwise use the flavor verbatim | ||
| active_flavor_dir = FLAVOR_TESTS_DIR / FLAVOR_TESTS_DIR_OVERRIDES.get(active_flavor, active_flavor) |
There was a problem hiding this comment.
In #722 I wanted to go all the way to feature-based testing anyway and reduce the flavor-based testing. I see here you already took the approach of exposing internal features (just like I did) so perhaps we can just get rid of the flavor based tests anyway?
tests/flavor/katello/postgresql_test.py already has logic that is more automated in tests/backup_test.py while tests/flavor/katello/images_test.py could also be easily written to look at enabled features.
The foreman-proxy-content are a bit more special, but that's not a concern for this particular PR.
b192d14 to
d8aa693
Compare
d8aa693 to
079aebc
Compare
| - webhooks | ||
| - hammer | ||
| - foreman-proxy | ||
|
|
There was a problem hiding this comment.
| - bootdisk | |
| - leapp | |
| - discovery | |
| - openscap | |
| - templates #foreman_templates | |
| - virt_who | |
I know some of them are not ready yet, but do we have an issue to track the missing features?
There was a problem hiding this comment.
That would mean also actually defining those features, and people would see them, try to enable them, and complain that it has no effect.
IMHO we should not.
|
SatelliteQE/robottelo#22356 (comment) So --remove-feature does not work when we use --flavor satellite ? |
You can't remove a feature that is part of the flavor, only those you added yourself. |
|
Running 
|
|
Where exactly did you expect to get the theme from? You need to use Satellite-based containers for that. |
So the |
Ideally, yes. I am still working on this aspect (see also #643) |
The satellite flavor provides a feature set matching what RPM-based Satellite ships by default, enabling proper downstream test coverage with foremanctl. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Map the satellite flavor to run katello flavor tests via FLAVOR_TEST_DIRS in conftest.py, so satellite deployments automatically pick up katello-specific test assertions. Add a satellite-tests CI job that deploys with --flavor satellite. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e5080c4 to
8543501
Compare
| - name: Setup environment | ||
| run: ./setup-environment | ||
| - name: Apply vendor overrides | ||
| run: cp -a vendor_overrides/satellite src/playbooks/_vendor_overrides |
There was a problem hiding this comment.
Is this copied to only test the vendor supplied flavors and with that we don't want to expose satellite flavor upstream? or did i misread something
There was a problem hiding this comment.
Correct. A "regular" (upstream) foremanctl deployment should never expose the Satellite flavor. Similarly, a Satellite deployment should not expose the Katello flavor (that's why this file fully overrides the choices).
I am still a bit torn whether I want this file to live here in the repo at the top level, or to put it as a fixture to the tests and let downstream ship an own version.
We could build foremanctl-overrides-<vendor> sub-packages from the files here, but not ship them in our repos, but then let downstreams ship them as needed. That's probably a question to @zjhuntin and @Odilhao how they want the downstream shipping to happen.
There was a problem hiding this comment.
I was thinking about features.d implementation, where downstream package just need to add the dropin, simiar to that maybe like src/flavors.d/satellite/metadata.obsah.yml drop in could work, but i am not sure how complex is that.
Also we could teach obsah to have a choices_directory from where it should pull flavor choices and we could differ that in upstream and downstream,
these are some thoughts i had, so added them here, but i don't want to block this PR on those as PR already looks neat to me.
There was a problem hiding this comment.
Going to the extreme: should upstream also be a vendor?
There was a problem hiding this comment.
Yeah, there are several ways we could make this easier/cleaner if we teach obsah a more explicit way to recognize overrides (right now I am merely abusing the fact that includes work and support partial overrides of previously defined stuff). If you want to experiment with this, please :)
There was a problem hiding this comment.
Going to the extreme: should upstream also be a vendor?
Not on a Friday, no. 😝
There was a problem hiding this comment.
theforeman/obsah#135 this is what i had in mind
There was a problem hiding this comment.
I am still a bit torn whether I want this file to live here in the repo at the top level, or to put it as a fixture to the tests and let downstream ship an own version.
We could build
foremanctl-overrides-<vendor>sub-packages from the files here, but not ship them in our repos, but then let downstreams ship them as needed.
Fixture vs. direct inclusion I am not sure it really matters, so maybe just start with the easiest solution.
As for the sub-package shipping, I don't think we should get into that game yet without more compelling use cases to do so.
There was a problem hiding this comment.
Yeah, there are several ways we could make this easier/cleaner if we teach obsah a more explicit way to recognize overrides (right now I am merely abusing the fact that includes work and support partial overrides of previously defined stuff). If you want to experiment with this, please :)
Everything is a vendor, even the vendor is the vendor. We are in Inception now. https://tenor.com/0xNi.gif
We could build foremanctl-overrides- sub-packages from the files here, but not ship them in our repos, but then let downstreams ship them as needed. That's probably a question to @zjhuntin and @Odilhao how they want the downstream shipping to happen.
I don't have any problem if we need to add one override file at one .d/ folder downstream to ensure that the override is present, since we will always have one opinionated package downstream.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis change adds Satellite flavor metadata, deployment overrides, installation and health checks, and configurable post-install messaging. It adds optional internal feature discovery and updates pytest fixtures and validation. It updates database expectations and feature markers. A new GitHub Actions job provisions Satellite environments, runs tests and smoker checks, archives reports, and participates in the aggregate test suite. Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/filter_plugins/foremanctl.py`:
- Around line 92-94: Update the output formatting in the feature-list loop to
emit the internal flag as numeric text, `0` or `1`, rather than Python’s `True`
or `False`, so consumers comparing the field to `'0'` parse available and
user-enabled features correctly.
In `@src/vars/flavors/satellite.yml`:
- Line 15: Update the satellite.yml feature declaration for theme-satellite so
it is enabled only when the container provides foreman_theme_satellite;
otherwise remove or disable the feature for nightly containers. Ensure
check_features and deployment roles do not advertise, install, or configure this
feature when the plugin is unavailable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04fb2ed7-b690-4ccc-a53b-78c68e6a996b
📒 Files selected for processing (13)
.github/workflows/test.ymlsrc/features.yamlsrc/filter_plugins/foremanctl.pysrc/playbooks/deploy/metadata.obsah.yamlsrc/roles/post_install/tasks/message.yamlsrc/vars/flavors/foreman-proxy-content.ymlsrc/vars/flavors/katello.ymlsrc/vars/flavors/satellite.ymltests/backup_test.pytests/conftest.pytests/features_test.pytests/httpd_test.pyvendor_overrides/satellite/deploy/metadata.obsah.yaml
| - remote-execution | ||
| - rh-cloud | ||
| - theme-satellite |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'theme-satellite|flavor_features|check_features|internal' src testsRepository: theforeman/foremanctl
Length of output: 46583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files ---'
git ls-files | rg '(^src/features\.|src/filter_plugins/|src/roles/checks|src/vars/flavors/satellite|src/vars/defaults|src/playbooks/deploy/_flavor_features|src/plugins|ansible|roles/.*/tasks/.*feature|checks)'
echo '--- src/features.yaml around theme-satellite ---'
sed -n '88,108p' src/features.yaml
echo '--- filter_plugins foremanctl.py ---'
sed -n '1,180p' src/filter_plugins/foremanctl.py
echo '--- checks roles files ---'
git ls-files src/roles/checks src/plugins src/**/*.yml | sort | rg 'check_.*feature|features|main\.yml|tasks/main'
echo '--- check_features references ---'
rg -n -C 5 "check_features|has_feature|theme-satellite|invalid_features|FEATURE_MAP|features.yaml|features\.yaml" src tests unit 2>/dev/null || true
echo '--- deploy plays include relevant snippets ---'
sed -n '1,120p' src/playbooks/deploy/deploy.yamlRepository: theforeman/foremanctl
Length of output: 46013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, re
root = 'src'
features = {}
for fname in ['features.yaml'] + sorted(glob_or_find('src/features.d/*.yaml') for _ in ()): # placeholder
pass
PYRepository: theforeman/foremanctl
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files ---'
git ls-files | rg '(^src/features\.|src/filter_plugins/|src/roles/checks|src/vars/flavors/satellite|src/vars/defaults|src/playbooks/deploy/_flavor_features|src/plugins|ansible|roles/.*/tasks/.*feature|checks)'
echo '--- src/features.yaml around theme-satellite ---'
sed -n '88,108p' src/features.yaml
echo '--- filter_plugins foremanctl.py ---'
sed -n '1,180p' src/filter_plugins/foremanctl.py
echo '--- checks roles files ---'
git ls-files src/roles/checks src/plugins src/**/*.yml | rg 'check_.*feature|features|main\.yml|tasks/main' || true
echo '--- check_features references ---'
rg -n -C 5 "check_features|has_feature|theme-satellite|invalid_features|FEATURE_MAP|features.yaml|features\.yaml" src tests 2>/dev/null || true
echo '--- deploy play snippets ---'
sed -n '1,120p' src/playbooks/deploy/deploy.yaml
echo '--- static list of feature mentions ---'
rg -n --hidden --glob '!*cache*' 'theme-satellite|has_feature|check_features|invalid_features|FEATURE_MAP' .Repository: theforeman/foremanctl
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- full check_features role ---'
sed -n '1,120p' src/roles/check_features/tasks/main.yaml
echo '--- task includes feature.yaml ---'
sed -n '1,80p' src/roles/foreman_proxy/tasks/feature.yaml
sed -n '1,100p' src/roles/foreman_proxy/tasks/feature/ansible.yaml
echo '--- search installed/configured plugin handling by name ---'
rg -n -C 4 'foreman_theme_satellite|theme_satellite|satellite theme|Satellite theme|internal_admin|enable_plugin|plugin_enabled|foreman-installer|foreman:.*plugins' src tests docs development 2>/dev/null || true
echo '--- Python feature map behavior for theme-satellite ---'
python3 - <<'PY'
import pathlib, yaml
root = pathlib.Path('src')
FEATURE_MAP = {}
FEATURE_MAP.update(yaml.safe_load((root / 'features.yaml').read_text()))
_features_d = root / 'features.d'
if _features_d.is_dir():
for _overlay in sorted(_features_d.glob('*.yaml')):
FEATURE_MAP.update(yaml.safe_load(_overlay.read_text()) or {})
def get_dependencies(features):
dependencies = set()
def add(feature):
if feature in dependencies:
return
dependencies.add(feature)
for dependency in FEATURE_MAP.get(feature, {}).get('dependencies', []):
add(dependency)
for feature in features:
add(feature)
return dependencies
enabled = ['satellite']
print('defined:', 'theme-satellite' in FEATURE_MAP)
print('meta:', FEATURE_MAP.get('theme-satellite'))
print('filters_for:', [FEATURE_MAP.get(f, {}).get('foreman', {}).get('plugin_name') for f in ['theme-satellite'] + get_dependencies(enabled)])
PYRepository: theforeman/foremanctl
Length of output: 33449
Condition theme-satellite on container features.
src/vars/flavors/satellite.yml enables theme-satellite unconditionally, but check_features only rejects unknown feature names, and deployment roles do not check Foreman capability before installing/configuring foreman_theme_satellite. If nightly containers lack this plugin, make the feature conditional or disable this fork so Satellite does not deploy a missing advertised feature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vars/flavors/satellite.yml` at line 15, Update the satellite.yml feature
declaration for theme-satellite so it is enabled only when the container
provides foreman_theme_satellite; otherwise remove or disable the feature for
nightly containers. Ensure check_features and deployment roles do not advertise,
install, or configure this feature when the plugin is unavailable.
8 participants
Why are you introducing these changes? (Problem description, related links)
What are the changes introduced in this pull request?
How to test this pull request
Steps to reproduce:
Checklist