Introduce a plain foreman flavor - #511
Conversation
48a7446 to
6aad15c
Compare
|
Our CI is currently not designed to run flavor specific tests https://github.com/theforeman/foremanctl/blob/master/.github/workflows/test.yml#L54, it adds all features but surely that won't make sense, as we want to only add relevent features for a flavor(ex: add bmc to plain foreman does not make any sense), Thinking out loud:- |
If you're talking about a GitHub Actions workflow: I think that does make sense. We probably can't test all combinations, but finding a good set is exactly what I think we should do. |
yes,and i think without flavor specific workflows we might end up with a single tests workflow which adds all available features(as of today) and will keep CI red |
a0a4d57 to
2c3a344
Compare
|
I'd still like to see this. Some PRs that are opened because of this, but can be merged already:
After that I'll rebase. |
2c3a344 to
ddb679e
Compare
|
The exception is that Lines 11 to 22 in 4d2d756 That means the parameters.yaml file doesn't exist.
|
Ah, I moved the file around and it tries to derive the root from the file location. |
ddb679e to
905eb13
Compare
| import pytest | ||
| import yaml | ||
|
|
||
| from tests.conftest import PARAMETERS_FILE |
There was a problem hiding this comment.
I wasn't sure about this, but I see tests/feature/foreman-proxy/base_test.py also imports FOREMAN_PROXY_PORT.
Then there's also this:
foremanctl/tests/migration_test.py
Lines 7 to 9 in 9ffe360
Should it be a global fixture?
There was a problem hiding this comment.
I think using
Line 20 in 9ffe360
There was a problem hiding this comment.
Are you suggesting to also update the migration test to use the constant?
There was a problem hiding this comment.
yes, looking at its used just to read params from the file , i think having a single defination for obsah state dir makes much more sense to me
There was a problem hiding this comment.
So question: should be have a global fixture or a constant? I'm leaning to avoid importing constants and instead rely on fixtures, but curious what you think.
There was a problem hiding this comment.
I think those both ways are valid, but personally for me i would choose constant as the state path is going to remain static throughout the test execution and does not need the overhead of using fixtures, but i might be wrong, thats my thesis
There was a problem hiding this comment.
I went with reusing the existing obsah_params fixture in #719 instead.
b669faa to
c2a38c5
Compare
|
I think this is largely ready, but we need to rethink CI a bit here. Adding IOP to plain Foreman implies adding Katello to it. While that's nice and all, it means we don't really test just Foreman. The additional proxy tests also all just fail. |
c2a38c5 to
0bc4e69
Compare
| assert "Index of /pulp/content/" in cmd.stdout | ||
|
|
||
|
|
||
| @pytest.mark.feature('pulp') |
There was a problem hiding this comment.
I am sure that ./foremanctl features will not show pulp as feature directly, its a internal feature, which can be found as has_feature filter but not as foremanctl features output, so these tests will be skipped always
There was a problem hiding this comment.
So we're finally hitting the point we discussed at the start when we talked about feature-based tsets: the test suite needs to know about internal features. Good catch.
There was a problem hiding this comment.
Yes, thats what forced me to use flavor directories for such cases https://github.com/theforeman/foremanctl/blob/master/tests/flavor/foreman-proxy-content/pulp_test.py ,
but yeah we can have a forge command to list internal features too
There was a problem hiding this comment.
At least the test suite is correctly flagging that:
ERROR tests/httpd_test.py::test_http_pulp_api_status - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_http_pulp_api_status>
ERROR tests/httpd_test.py::test_https_pulp_api_status - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_https_pulp_api_status>
ERROR tests/httpd_test.py::test_http_pulp_content - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_http_pulp_content>
ERROR tests/httpd_test.py::test_https_pulp_content - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_https_pulp_content>
ERROR tests/httpd_test.py::test_https_pulp_auth - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_https_pulp_auth>
ERROR tests/httpd_test.py::test_https_pypi_endpoint - pytest.PytestConfigWarning: Invalid feature(s) {'pulp'} on <Function test_https_pypi_endpoint>
| - "enabled_features | has_feature('foreman')" | ||
| - "enabled_features | has_feature('katello')" |
There was a problem hiding this comment.
I missed there's now a built in candlepin feature that should be used.
| - "enabled_features | has_feature('foreman')" | |
| - "enabled_features | has_feature('katello')" | |
| - "enabled_features | has_feature('candlepin')" |
0bc4e69 to
06b8f53
Compare
|
Right now all the prerequisites I could think of are merged. This now needs testing; both manual and CI. |
This moves Candlepin and Pulp behind the feature flags. Candlepin is only deployed on Foreman with Katello servers while Pulp is deployed on Katello enabled servers.
06b8f53 to
b596aef
Compare
Why are you introducing these changes? (Problem description, related links)
Today Foreman can be deployed without Katello on both RPM and Debian installations and this allows users to mirror that.
What are the changes introduced in this pull request?
The flavor itself is rather trivial, but there are some changes made to only deploy certain roles depending on the features enabled. That is likely incomplete. In particular, Apache is not yet modified to only deploy the required services. In the future when a plain foreman-proxy role is introduced then that will need adjustment.
There are also heavy testing changes, heavily based on #509 which in turn is based on #508. I'm wondering how much of those commits already belong there.
Right now the test suite fails because the
foremanfeature is only set in the flavor and not in the parameters.It's also important to look at how this will play into #431.
How to test this pull request
Steps to reproduce:
foremanctl deploy --flavor foremanChecklist