[3006.x] fix(saltclass): don't lose nested classes and states in recursion #66918

baby-gnu · 2024-09-23T20:05:05Z

What does this PR do?

This PR keep all the classes and states from recursive expansion of saltclass classes.

What issues does this PR fix or reference?

Fixes #58969
Fixes #63933

Previous Behavior

Only states from top level classes were kept.

New Behavior

All states from all classes are used, in the correct order.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.

See GitHub's page on GPG signing for more information about signing commits with GPG.

tools/precommit/docstrings.py

twangboy · 2024-09-23T21:41:51Z

We'll need a changelog and some tests for this.

baby-gnu · 2024-09-24T12:11:57Z

We'll need a changelog and some tests for this.

Ok, the changelog is quite easy, I'll need to document for testing.

baby-gnu · 2024-09-27T06:56:20Z

We'll need a changelog and some tests for this.

@twangboy: I added tests for both issues:

Errors when passing the tests before my PRnox -e 'test-3(coverage=False)' -- tests/pytests/unit/pillar/test_saltclass.py

nox > Running session test-3(coverage=False)
nox > Re-using existing virtual environment at .nox/test-3-coverage-false.
nox > Session test-3(coverage=False) was successful.
nox > Running session test-parametrized-3(crypto=None, transport='zeromq', coverage=False)
nox > Re-using existing virtual environment at .nox/test-parametrized-3-crypto-none-transport-zeromq-coverage-false.
nox > python -m pip install --progress-bar=off -U setuptools pip wheel
nox > python -m pip install --progress-bar=off -r requirements/static/ci/py3.10/linux.txt
nox > python -m pytest --rootdir /src --log-file-level=debug --show-capture=no -ra -s -vv --showlocals --log-file=/src/artifacts/logs/runtests-20240927064217.175879.log --transport=zeromq tests/pytests/unit/pillar/test_saltclass.py
<frozen importlib._bootstrap>:914: ImportWarning: TornadoImporter.find_spec() not found; falling back to find_module()
<frozen importlib._bootstrap>:914: ImportWarning: NaclImporter.find_spec() not found; falling back to find_module()
============================= test session starts ==============================
platform linux -- Python 3.10.14, pytest-8.1.1, pluggy-1.4.0 -- /src/.nox/test-parametrized-3-crypto-none-transport-zeromq-coverage-false/bin/python
cachedir: .pytest_cache
max open files: soft=1048576; hard=1048576
salt-transport: zeromq
rootdir: /src
configfile: pytest.ini
plugins: skip-markers-1.5.2, httpserver-1.0.8, helpers-namespace-2021.4.29, flaky-3.8.1, salt-factories-1.0.1, timeout-2.3.1, pyfakefs-5.3.1, system-statistics-1.0.2, custom-exit-code-0.3.0, subtests-0.4.0, shell-utilities-1.8.0
collecting ... collected 7 items

tests/pytests/unit/pillar/test_saltclass.py::test_classes_order FAILED
tests/pytests/unit/pillar/test_saltclass.py::test_list_expansion_succeeds PASSED
tests/pytests/unit/pillar/test_saltclass.py::test_node_override_classes_scalars PASSED
tests/pytests/unit/pillar/test_saltclass.py::test_node_override_classes_scalar_in_dict FAILED
tests/pytests/unit/pillar/test_saltclass.py::test_node_override_classes_list_in_dict PASSED
tests/pytests/unit/pillar/test_saltclass.py::test_list_in_dict_no_duplication FAILED
tests/pytests/unit/pillar/test_saltclass.py::test_nested_classes_has_pillars PASSED

=================================== FAILURES ===================================
______________________________ test_classes_order ______________________________

temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_classes_order0/saltclass/examples')

    def test_classes_order(temp_saltclass_tree):
        """
        Classes must be correctly ordered.
    
        See: https://github.com/saltstack/salt/issues/58969
        """
        expected_ret = [
            "default.users.foo",
            "default.users",
            "default.motd",
            "default.empty",
            "default",
            "roles.app",
            "roles.nginx",
        ]
        fake_args = {"path": str(temp_saltclass_tree)}
        fake_pillar = {}
        fake_minion_id = "fake_id"
        try:
            full_ret = saltclass.ext_pillar(fake_minion_id, fake_pillar, fake_args)
            parsed_ret = full_ret["__saltclass__"]["classes"]
        # Fail the test if we hit our NoneType error
        except TypeError as err:
            pytest.fail(err)
        # Else give the parsed content result
>       assert expected_ret == parsed_ret
E       AssertionError: assert ['default.users.foo', 'default.users', 'default.motd', 'default.empty', 'default', 'roles.app', 'roles.nginx'] == ['default.users', 'default.motd', 'default.empty', 'default', 'roles.app', 'roles.nginx']
E         
E         At index 0 diff: 'default.users.foo' != 'default.users'
E         Left contains one more item: 'roles.nginx'
E         
E         Full diff:
E           [
E         +     'default.users.foo',
E               'default.users',
E               'default.motd',
E               'default.empty',
E               'default',
E               'roles.app',
E               'roles.nginx',
E           ]

expected_ret = ['default.users.foo',
 'default.users',
 'default.motd',
 'default.empty',
 'default',
 'roles.app',
 'roles.nginx']
fake_args  = {'path': '/tmp/pytest-of-root/pytest-77/test_classes_order0/saltclass/examples'}
fake_minion_id = 'fake_id'
fake_pillar = {'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from nginx',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
full_ret   = {'__saltclass__': {'classes': ['default.users',
                               'default.motd',
                               'default.empty',
                               'default',
                               'roles.app',
                               'roles.nginx'],
                   'environment': 'base',
                   'nodename': 'fake_id',
                   'states': ['users',
                              'motd',
                              'default',
                              'app',
                              'nginx',
                              'minion_node']},
 'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from minion_node',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
parsed_ret = ['default.users',
 'default.motd',
 'default.empty',
 'default',
 'roles.app',
 'roles.nginx']
temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_classes_order0/saltclass/examples')

tests/pytests/unit/pillar/test_saltclass.py:174: AssertionError
__________________ test_node_override_classes_scalar_in_dict ___________________

temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_node_override_classes_sca1/saltclass/examples')

    def test_node_override_classes_scalar_in_dict(temp_saltclass_tree):
        """
        Scalars defined in `dict` pillars defined in a node definition must override the
        same dict definition from classes.
    
        See: https://github.com/saltstack/salt/issues/63933
        """
        expected_ret = "from minion_node"
        fake_args = {"path": str(temp_saltclass_tree)}
        fake_pillar = {}
        fake_minion_id = "fake_id"
        try:
            full_ret = saltclass.ext_pillar(fake_minion_id, fake_pillar, fake_args)
            parsed_ret = full_ret["test_dict"]["a_scalar"]
        # Fail the test if we hit our NoneType error
        except TypeError as err:
            pytest.fail(err)
        # Else give the parsed content result
>       assert expected_ret == parsed_ret
E       AssertionError: assert 'from minion_node' == 'from users'
E         
E         - from users
E         + from minion_node

expected_ret = 'from minion_node'
fake_args  = {'path': '/tmp/pytest-of-root/pytest-77/test_node_override_classes_sca1/saltclass/examples'}
fake_minion_id = 'fake_id'
fake_pillar = {'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from nginx',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
full_ret   = {'__saltclass__': {'classes': ['default.users',
                               'default.motd',
                               'default.empty',
                               'default',
                               'roles.app',
                               'roles.nginx'],
                   'environment': 'base',
                   'nodename': 'fake_id',
                   'states': ['users',
                              'motd',
                              'default',
                              'app',
                              'nginx',
                              'minion_node']},
 'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from minion_node',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
parsed_ret = 'from users'
temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_node_override_classes_sca1/saltclass/examples')

tests/pytests/unit/pillar/test_saltclass.py:231: AssertionError
_______________________ test_list_in_dict_no_duplication _______________________

temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_list_in_dict_no_duplicati0/saltclass/examples')

    def test_list_in_dict_no_duplication(temp_saltclass_tree):
        """
        `list` under a `dict` in pillar must not be duplicated.
    
        See:
        """
        expected_ret = ["element1", "element2"]
        fake_args = {"path": str(temp_saltclass_tree)}
        fake_pillar = {}
        fake_minion_id = "fake_id"
        try:
            full_ret = saltclass.ext_pillar(fake_minion_id, fake_pillar, fake_args)
            parsed_ret = full_ret["test_dict"]["a_list"]
        # Fail the test if we hit our NoneType error
        except TypeError as err:
            pytest.fail(err)
        # Else give the parsed content result
>       assert expected_ret == parsed_ret
E       AssertionError: assert ['element1', 'element2'] == ['element1', 'element2', 'element1', 'element2']
E         
E         Right contains 2 more items, first extra item: 'element1'
E         
E         Full diff:
E           [
E               'element1',
E               'element2',
E         -     'element1',
E         -     'element2',
E           ]

expected_ret = ['element1', 'element2']
fake_args  = {'path': '/tmp/pytest-of-root/pytest-77/test_list_in_dict_no_duplicati0/saltclass/examples'}
fake_minion_id = 'fake_id'
fake_pillar = {'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from nginx',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
full_ret   = {'__saltclass__': {'classes': ['default.users',
                               'default.motd',
                               'default.empty',
                               'default',
                               'roles.app',
                               'roles.nginx'],
                   'environment': 'base',
                   'nodename': 'fake_id',
                   'states': ['users',
                              'motd',
                              'default',
                              'app',
                              'nginx',
                              'minion_node']},
 'default': {'network': {'dns': None,
                         'ntp': {'srv1': '192.168.10.10',
                                 'srv2': '192.168.10.20'}},
             'ntp': {'srv1': '192.168.20.10'}},
 'global_scalar': 'from minion_node',
 'nginx_scalar': 'from nginx',
 'test_dict': {'a_list': ['element1', 'element2', 'element1', 'element2'],
               'a_scalar': 'from users'},
 'test_list': [{'a': '192.168.10.10'}, '192.168.10.20']}
parsed_ret = ['element1', 'element2', 'element1', 'element2']
temp_saltclass_tree = PosixPath('/tmp/pytest-of-root/pytest-77/test_list_in_dict_no_duplicati0/saltclass/examples')

tests/pytests/unit/pillar/test_saltclass.py:272: AssertionError
=========================== short test summary info ============================
FAILED tests/pytests/unit/pillar/test_saltclass.py::test_classes_order - Asse...
FAILED tests/pytests/unit/pillar/test_saltclass.py::test_node_override_classes_scalar_in_dict
FAILED tests/pytests/unit/pillar/test_saltclass.py::test_list_in_dict_no_duplication
========================= 3 failed, 4 passed in 1.09s ==========================
nox > Command python -m pytest --rootdir /src --log-file-level=debug --show-capture=no -ra -s -vv --showlocals --log-file=/src/artifacts/logs/runtests-20240927064217.175879.log --transport=zeromq tests/pytests/unit/pillar/test_saltclass.py failed with exit code 1
nox > Session test-parametrized-3(crypto=None, transport='zeromq', coverage=False) failed.
nox > Ran multiple sessions:
nox > * test-3(coverage=False): success
nox > * test-parametrized-3(crypto=None, transport='zeromq', coverage=False): failed

baby-gnu · 2024-09-30T22:48:11Z

Thanks @twangboy, should I rebase the branch over 3006.x from time to time until it get merge?

Fixes: saltstack#58969

A pillars dict in a minion node definition was not overridding correctly the same dict in classes. * salt/utils/saltclass.py (expanded_dict_from_minion): do not pass a reference to minion dict or it will be overridden by classes during expansion. Fixes: saltstack#63933

baby-gnu · 2024-09-30T22:53:17Z

Thanks @twangboy, should I rebase the branch over 3006.x from time to time until it get merge?

I reword my commit message to fix typo as you did in the title 👍

baby-gnu requested a review from a team as a code owner September 23, 2024 20:05

salt-project-bot-prod-environment bot changed the title ~~fix(saltclass): don't loose nested classes and states in recursion~~ [3006.x] fix(saltclass): don't loose nested classes and states in recursion Sep 23, 2024

baby-gnu temporarily deployed to ci September 23, 2024 20:22 — with GitHub Actions Inactive

baby-gnu temporarily deployed to ci September 23, 2024 20:23 — with GitHub Actions Inactive

twangboy reviewed Sep 23, 2024

View reviewed changes

tools/precommit/docstrings.py Show resolved Hide resolved

baby-gnu mentioned this pull request Sep 26, 2024

[BUG] saltclass node pillar does not override classes pillar dict #63933

Open

baby-gnu force-pushed the fix/saltclass-nested-states branch from 0af0ff8 to 2d967b6 Compare September 26, 2024 13:22

baby-gnu temporarily deployed to ci September 26, 2024 13:46 — with GitHub Actions Inactive

twangboy previously approved these changes Sep 26, 2024

View reviewed changes

baby-gnu dismissed twangboy’s stale review via 8c4d676 September 26, 2024 19:51

baby-gnu force-pushed the fix/saltclass-nested-states branch 3 times, most recently from 316a5b6 to 45eab43 Compare September 26, 2024 19:58

baby-gnu temporarily deployed to ci September 26, 2024 20:18 — with GitHub Actions Inactive

twangboy changed the title ~~[3006.x] fix(saltclass): don't loose nested classes and states in recursion~~ [3006.x] fix(saltclass): don't lose nested classes and states in recursion Sep 30, 2024

twangboy previously approved these changes Sep 30, 2024

View reviewed changes

baby-gnu added 4 commits October 1, 2024 00:49

Add test for saltclass nested classes expansion

7e829de

fix(saltclass): don't lose nested classes and states in recursion

54e9225

Fixes: saltstack#58969

Remove expand_classes_in_order from missing_docstrings

6476468

baby-gnu dismissed twangboy’s stale review via 6476468 September 30, 2024 22:52

baby-gnu force-pushed the fix/saltclass-nested-states branch from 45eab43 to 6476468 Compare September 30, 2024 22:52

baby-gnu temporarily deployed to ci September 30, 2024 23:11 — with GitHub Actions Inactive

dwoz requested a review from twangboy October 1, 2024 00:49

twangboy approved these changes Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3006.x] fix(saltclass): don't lose nested classes and states in recursion #66918

[3006.x] fix(saltclass): don't lose nested classes and states in recursion #66918

baby-gnu commented Sep 23, 2024 •

edited

Loading

twangboy commented Sep 23, 2024

baby-gnu commented Sep 24, 2024

baby-gnu commented Sep 27, 2024

baby-gnu commented Sep 30, 2024

baby-gnu commented Sep 30, 2024

[3006.x] fix(saltclass): don't lose nested classes and states in recursion #66918

Are you sure you want to change the base?

[3006.x] fix(saltclass): don't lose nested classes and states in recursion #66918

Conversation

baby-gnu commented Sep 23, 2024 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Merge requirements satisfied?

Commits signed with GPG?

twangboy commented Sep 23, 2024

baby-gnu commented Sep 24, 2024

baby-gnu commented Sep 27, 2024

baby-gnu commented Sep 30, 2024

baby-gnu commented Sep 30, 2024

baby-gnu commented Sep 23, 2024 •

edited

Loading