-
Notifications
You must be signed in to change notification settings - Fork 46
Satellite flavor #726
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
Satellite flavor #726
Changes from all commits
f4ff48d
4289f20
b58c348
87e71f4
45d67dd
6a3d0a1
5bd0dad
9940cf3
6024c99
1c9b409
07eace3
d9672ee
4da5bcf
8543501
f5676b4
e09da29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,3 +26,4 @@ variables: | |
| include: | ||
| - _flavor_features | ||
| - _flavors/katello | ||
| - _vendor_overrides/deploy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| flavor_features: | ||
| - foreman-proxy | ||
| - pulp | ||
| - content/rpm | ||
| - content/deb | ||
| - content/container | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||||||||||||||||
| --- | ||||||||||||||||||||
| flavor_features: | ||||||||||||||||||||
| - foreman | ||||||||||||||||||||
| - katello | ||||||||||||||||||||
|
Gauravtalreja1 marked this conversation as resolved.
|
||||||||||||||||||||
| - pulp | ||||||||||||||||||||
| - candlepin | ||||||||||||||||||||
| - content/ansible | ||||||||||||||||||||
| - content/container | ||||||||||||||||||||
| - content/rpm | ||||||||||||||||||||
| - ansible | ||||||||||||||||||||
| - azure-rm | ||||||||||||||||||||
| - remote-execution | ||||||||||||||||||||
| - rh-cloud | ||||||||||||||||||||
| - theme-satellite | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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
🤖 Prompt for AI Agents
Gauravtalreja1 marked this conversation as resolved.
|
||||||||||||||||||||
| - webhooks | ||||||||||||||||||||
| - hammer | ||||||||||||||||||||
| - foreman-proxy | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I know some of them are not ready yet, but do we have an issue to track the missing features?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||
| checks_to_execute: | ||||||||||||||||||||
| - check_features | ||||||||||||||||||||
| - check_hostname | ||||||||||||||||||||
| - check_database_connection | ||||||||||||||||||||
| - check_system_requirements | ||||||||||||||||||||
| - check_podman_network_backend | ||||||||||||||||||||
|
|
||||||||||||||||||||
| health_checks_to_execute: | ||||||||||||||||||||
| - check_hostname | ||||||||||||||||||||
| - check_database_connection | ||||||||||||||||||||
| - check_services | ||||||||||||||||||||
| - check_foreman_api | ||||||||||||||||||||
| - check_foreman_tasks | ||||||||||||||||||||
| - check_host_facts_count | ||||||||||||||||||||
| - check_duplicate_permissions | ||||||||||||||||||||
|
|
||||||||||||||||||||
| post_install_message_enabled: true | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,9 @@ | |
| OBSAH_STATE = os.environ.get('OBSAH_STATE', '.var/lib/foremanctl') | ||
| PARAMETERS_FILE = os.path.join(OBSAH_STATE, 'parameters.yaml') | ||
| FLAVOR_TESTS_DIR = py.path.local(__file__).dirpath() / 'flavor' | ||
| FLAVOR_TESTS_DIR_OVERRIDES = { | ||
| 'satellite': 'katello', | ||
| } | ||
| FOREMAN_PROXY_PORT = 8443 | ||
|
|
||
|
|
||
|
|
@@ -32,18 +35,27 @@ def features(self): | |
| # FEATURE STATE DESCRIPTION | ||
| # $feature enabled/available $description | ||
| output = subprocess.check_output(['./foremanctl', 'features'], cwd=self._config.rootdir, | ||
| universal_newlines=True) | ||
| universal_newlines=True, | ||
| env=os.environ | {'FOREMANCTL_FEATURES_LIST_INTERNAL': 'true'}) | ||
| lines = output.splitlines(keepends=False) | ||
| # feature, status, description | ||
| return [line.split(None, 2) for line in lines[1:]] | ||
| # feature, status, internal, description | ||
| return [line.split(None, 3) for line in lines[1:]] | ||
|
|
||
| @cached_property | ||
| def all_available_features(self): | ||
| return set(feature for feature, _status, _internal, _desc in self.features) | ||
|
|
||
| @cached_property | ||
| def available_features(self): | ||
| return set(feature for feature, _status, _desc in self.features) | ||
| return set(feature for feature, _status, internal, _desc in self.features if internal == '0') | ||
|
|
||
| @cached_property | ||
| def enabled_features(self): | ||
| return set(feature for feature, status, _desc in self.features if status == 'enabled') | ||
| return set(feature for feature, status, internal, _desc in self.features if status == 'enabled') | ||
|
|
||
| @cached_property | ||
| def user_enabled_features(self): | ||
| return set(feature for feature, status, internal, _desc in self.features if status == 'enabled' and internal == '0') | ||
|
|
||
| @cached_property | ||
| def flavor(self): | ||
|
|
@@ -66,6 +78,11 @@ def enabled_features(pytestconfig): | |
| return pytestconfig.user_parameters.enabled_features | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def user_enabled_features(pytestconfig): | ||
| return pytestconfig.user_parameters.user_enabled_features | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def available_features(pytestconfig): | ||
| return pytestconfig.user_parameters.available_features | ||
|
|
@@ -286,7 +303,8 @@ def pytest_configure(config): | |
|
|
||
| def pytest_collection_modifyitems(config, items): | ||
| 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) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this means we can't have a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
The |
||
|
|
||
| deselected = [] | ||
| selected = [] | ||
|
|
@@ -317,7 +335,7 @@ def pytest_collection_modifyitems(config, items): | |
| def pytest_runtest_setup(item): | ||
| feature_markers = set(mark.args[0] for mark in item.iter_markers(name="feature")) | ||
| if feature_markers: | ||
| invalid_features = feature_markers - item.config.user_parameters.available_features | ||
| invalid_features = feature_markers - item.config.user_parameters.all_available_features | ||
| if invalid_features: | ||
| raise pytest.PytestConfigWarning(f"Invalid feature(s) {invalid_features!r} on {item}") | ||
| missing = feature_markers - item.config.user_parameters.enabled_features | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| variables: | ||
| flavor: | ||
| choices: | ||
| - satellite |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about
features.dimplementation, where downstream package just need to add the dropin, simiar to that maybe likesrc/flavors.d/satellite/metadata.obsah.ymldrop in could work, but i am not sure how complex is that.Also we could teach obsah to have a
choices_directoryfrom 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to the extreme: should upstream also be a vendor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on a Friday, no. 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theforeman/obsah#135 this is what i had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is a vendor, even the vendor is the vendor. We are in Inception now. https://tenor.com/0xNi.gif
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
opinionatedpackage downstream.