From 9e353f85118bd746f75176244c26661de6d26d66 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 31 Jan 2024 12:53:05 +0000 Subject: [PATCH 1/9] Fix docs builds --- .github/workflows/build-docs.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-docs.yml b/.github/workflows/build-docs.yml index e99c84ef303..53bc06fbd62 100644 --- a/.github/workflows/build-docs.yml +++ b/.github/workflows/build-docs.yml @@ -76,10 +76,10 @@ jobs: - name: Prepare Docs Build run: | - git clone https://gitlab.com/saltstack/open/docs/builddocs.git .builddocs + git clone https://gitlab.com/saltstack/open/docs/builddocs-fonts.git .builddocs-fonts sudo mkdir -p /usr/share/fonts/truetype /usr/share/fonts/opentype - sudo cp -rfv .builddocs/builddocs/files/fonts/truetype/*.ttf /usr/share/fonts/truetype/ - sudo cp -rfv .builddocs/builddocs/files/fonts/opentype/*.otf /usr/share/fonts/opentype/ + sudo cp -rfv .builddocs-fonts/*.ttf /usr/share/fonts/truetype/ + sudo cp -rfv .builddocs-fonts/*.otf /usr/share/fonts/opentype/ sudo fc-cache -f -v - name: Build Documentation (${{ matrix.docs-output }}) From 56b0646f2bced5aef15486735e82450b2a001e18 Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Sun, 28 Jan 2024 20:23:06 +0700 Subject: [PATCH 2/9] Allow synced modules to override salt extensions --- salt/loader/__init__.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index 38382a32a51..efb0b854131 100644 --- a/salt/loader/__init__.py +++ b/salt/loader/__init__.py @@ -130,17 +130,18 @@ def _module_dirs( ): if tag is None: tag = ext_type - sys_types = os.path.join(base_path or str(SALT_BASE_PATH), int_type or ext_type) - return_types = [sys_types] - if opts.get("extension_modules"): - ext_types = os.path.join(opts["extension_modules"], ext_type) - return_types.insert(0, ext_types) + sys_types = [os.path.join(base_path or str(SALT_BASE_PATH), int_type or ext_type)] - if not sys_types.startswith(SALT_INTERNAL_LOADERS_PATHS): + if opts.get("extension_modules"): + ext_types = [os.path.join(opts["extension_modules"], ext_type)] + else: + ext_types = [] + + if not sys_types[0].startswith(SALT_INTERNAL_LOADERS_PATHS): raise RuntimeError( "{!r} is not considered a salt internal loader path. If this " "is a new loader being added, please also add it to " - "{}.SALT_INTERNAL_LOADERS_PATHS.".format(sys_types, __name__) + "{}.SALT_INTERNAL_LOADERS_PATHS.".format(sys_types[0], __name__) ) ext_type_types = [] @@ -250,7 +251,7 @@ def _module_dirs( if os.path.isdir(maybe_dir): cli_module_dirs.insert(0, maybe_dir) - return cli_module_dirs + ext_type_types + return_types + return cli_module_dirs + ext_types + ext_type_types + sys_types def minion_mods( From be83897d23d5ececc413f3d7e8d7349ef82cb316 Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Mon, 29 Jan 2024 15:51:16 +0700 Subject: [PATCH 3/9] Add opt-out feature toggle --- salt/loader/__init__.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index efb0b854131..c999dcfec5d 100644 --- a/salt/loader/__init__.py +++ b/salt/loader/__init__.py @@ -28,6 +28,7 @@ import salt.utils.platform import salt.utils.stringutils import salt.utils.versions from salt.exceptions import LoaderError +from salt.features import features from salt.template import check_render_pipe_str from salt.utils import entrypoints @@ -251,7 +252,15 @@ def _module_dirs( if os.path.isdir(maybe_dir): cli_module_dirs.insert(0, maybe_dir) - return cli_module_dirs + ext_types + ext_type_types + sys_types + if features.get("enable_deprecated_module_search_path_priority", False): + salt.utils.versions.warn_until( + 3008, + "The old module search path priority will be removed in Salt Argon. " + "For more information see https://github.com/saltstack/salt/pull/65938.", + ) + return cli_module_dirs + ext_type_types + ext_types + sys_types + else: + return cli_module_dirs + ext_types + ext_type_types + sys_types def minion_mods( From 85cce64de92aa0ab2f8b7993e5f28643af02bb2e Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Mon, 29 Jan 2024 15:51:50 +0700 Subject: [PATCH 4/9] Add functional test --- .../pytests/functional/loader/test_loader.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/pytests/functional/loader/test_loader.py b/tests/pytests/functional/loader/test_loader.py index 963d33f59c3..5d1e7989f50 100644 --- a/tests/pytests/functional/loader/test_loader.py +++ b/tests/pytests/functional/loader/test_loader.py @@ -1,4 +1,5 @@ import json +import shutil import pytest @@ -26,6 +27,50 @@ def venv(tmp_path): yield _venv +@pytest.fixture +def module_dirs(tmp_path): + module_dir = tmp_path / "module-dir-base" + (module_dir / "modules").mkdir(parents=True) + try: + yield [str(module_dir)] + finally: + shutil.rmtree(str(module_dir), ignore_errors=True) + + +def test_module_dirs_priority(venv, salt_extension, salt_minion_factory, module_dirs): + # Install our extension into the virtualenv + venv.install(str(salt_extension.srcdir)) + installed_packages = venv.get_installed_packages() + assert salt_extension.name in installed_packages + code = """ + import sys + import json + import salt._logging + import salt.loader + + minion_config = json.loads(sys.stdin.read()) + salt._logging.set_logging_options_dict(minion_config) + salt._logging.setup_logging() + mod_dirs = salt.loader._module_dirs(minion_config, "modules", "module") + print(json.dumps(mod_dirs)) + """ + opts = salt_minion_factory.config.copy() + opts["module_dirs"] = module_dirs + ret = venv.run_code(code, input=json.dumps(opts)) + module_dirs_return = json.loads(ret.stdout) + assert len(module_dirs_return) == 5 + for i, tail in enumerate( + [ + "/module-dir-base/modules", + "/var/cache/salt/minion/extmods/modules", + "/module-dir-base", + "/site-packages/salt_ext_loader_test/modules", + "/site-packages/salt/modules", + ] + ): + assert module_dirs_return[i].endswith(tail) + + def test_new_entry_points_passing_module(venv, salt_extension, salt_minion_factory): # Install our extension into the virtualenv venv.install(str(salt_extension.srcdir)) From 94047ee5967d2e8358e077d437a875d2ee87d889 Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Mon, 29 Jan 2024 15:57:38 +0700 Subject: [PATCH 5/9] Add changelog entry --- changelog/65938.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/65938.changed.md diff --git a/changelog/65938.changed.md b/changelog/65938.changed.md new file mode 100644 index 00000000000..a96e39b485d --- /dev/null +++ b/changelog/65938.changed.md @@ -0,0 +1 @@ +Change module search path priority, so Salt extensions can be overridden by syncable modules and module_dirs. You can switch back to the old logic by setting features.enable_deprecated_module_search_path_priority to true, but it will be removed in Salt 3008. From 4769ae02365779229d5669096f393b63544ad1d3 Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Tue, 30 Jan 2024 18:03:47 +0700 Subject: [PATCH 6/9] Address the code review comments --- .../pytests/functional/loader/test_loader.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/tests/pytests/functional/loader/test_loader.py b/tests/pytests/functional/loader/test_loader.py index 5d1e7989f50..97e8843d4cf 100644 --- a/tests/pytests/functional/loader/test_loader.py +++ b/tests/pytests/functional/loader/test_loader.py @@ -1,5 +1,4 @@ import json -import shutil import pytest @@ -30,14 +29,11 @@ def venv(tmp_path): @pytest.fixture def module_dirs(tmp_path): module_dir = tmp_path / "module-dir-base" - (module_dir / "modules").mkdir(parents=True) - try: - yield [str(module_dir)] - finally: - shutil.rmtree(str(module_dir), ignore_errors=True) + module_dir.joinpath("modules").mkdir(parents=True) + return [str(module_dir)] -def test_module_dirs_priority(venv, salt_extension, salt_minion_factory, module_dirs): +def test_module_dirs_priority(venv, salt_extension, minion_opts, module_dirs): # Install our extension into the virtualenv venv.install(str(salt_extension.srcdir)) installed_packages = venv.get_installed_packages() @@ -54,9 +50,8 @@ def test_module_dirs_priority(venv, salt_extension, salt_minion_factory, module_ mod_dirs = salt.loader._module_dirs(minion_config, "modules", "module") print(json.dumps(mod_dirs)) """ - opts = salt_minion_factory.config.copy() - opts["module_dirs"] = module_dirs - ret = venv.run_code(code, input=json.dumps(opts)) + minion_opts["module_dirs"] = module_dirs + ret = venv.run_code(code, input=json.dumps(minion_opts)) module_dirs_return = json.loads(ret.stdout) assert len(module_dirs_return) == 5 for i, tail in enumerate( @@ -68,7 +63,9 @@ def test_module_dirs_priority(venv, salt_extension, salt_minion_factory, module_ "/site-packages/salt/modules", ] ): - assert module_dirs_return[i].endswith(tail) + assert module_dirs_return[i].endswith( + tail + ), f"{module_dirs_return[i]} does not end with {tail}" def test_new_entry_points_passing_module(venv, salt_extension, salt_minion_factory): From a14e899ffb4206717b1ff9dbefcdf05c1d9bbf51 Mon Sep 17 00:00:00 2001 From: Max Arnold Date: Tue, 30 Jan 2024 18:54:52 +0700 Subject: [PATCH 7/9] Use the "features" option directly, add test --- salt/loader/__init__.py | 7 ++++--- tests/pytests/functional/loader/test_loader.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index c999dcfec5d..f8feedfe727 100644 --- a/salt/loader/__init__.py +++ b/salt/loader/__init__.py @@ -28,7 +28,6 @@ import salt.utils.platform import salt.utils.stringutils import salt.utils.versions from salt.exceptions import LoaderError -from salt.features import features from salt.template import check_render_pipe_str from salt.utils import entrypoints @@ -252,10 +251,12 @@ def _module_dirs( if os.path.isdir(maybe_dir): cli_module_dirs.insert(0, maybe_dir) - if features.get("enable_deprecated_module_search_path_priority", False): + if opts.get("features", {}).get( + "enable_deprecated_module_search_path_priority", False + ): salt.utils.versions.warn_until( 3008, - "The old module search path priority will be removed in Salt Argon. " + "The old module search path priority will be removed in Salt 3008. " "For more information see https://github.com/saltstack/salt/pull/65938.", ) return cli_module_dirs + ext_type_types + ext_types + sys_types diff --git a/tests/pytests/functional/loader/test_loader.py b/tests/pytests/functional/loader/test_loader.py index 97e8843d4cf..33b6bd793d3 100644 --- a/tests/pytests/functional/loader/test_loader.py +++ b/tests/pytests/functional/loader/test_loader.py @@ -67,6 +67,24 @@ def test_module_dirs_priority(venv, salt_extension, minion_opts, module_dirs): tail ), f"{module_dirs_return[i]} does not end with {tail}" + # Test the deprecated mode as well + minion_opts["features"] = {"enable_deprecated_module_search_path_priority": True} + ret = venv.run_code(code, input=json.dumps(minion_opts)) + module_dirs_return = json.loads(ret.stdout) + assert len(module_dirs_return) == 5 + for i, tail in enumerate( + [ + "/module-dir-base/modules", + "/module-dir-base", + "/site-packages/salt_ext_loader_test/modules", + "/var/cache/salt/minion/extmods/modules", + "/site-packages/salt/modules", + ] + ): + assert module_dirs_return[i].endswith( + tail + ), f"{module_dirs_return[i]} does not end with {tail}" + def test_new_entry_points_passing_module(venv, salt_extension, salt_minion_factory): # Install our extension into the virtualenv From 3dc0a2ca30d4fb7a6c09772596a5651da457968a Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 31 Jan 2024 13:07:24 +0000 Subject: [PATCH 8/9] Fix the copy path --- .github/workflows/build-docs.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-docs.yml b/.github/workflows/build-docs.yml index 53bc06fbd62..415cdfc7d19 100644 --- a/.github/workflows/build-docs.yml +++ b/.github/workflows/build-docs.yml @@ -78,8 +78,8 @@ jobs: run: | git clone https://gitlab.com/saltstack/open/docs/builddocs-fonts.git .builddocs-fonts sudo mkdir -p /usr/share/fonts/truetype /usr/share/fonts/opentype - sudo cp -rfv .builddocs-fonts/*.ttf /usr/share/fonts/truetype/ - sudo cp -rfv .builddocs-fonts/*.otf /usr/share/fonts/opentype/ + sudo cp -rfv .builddocs-fonts/truetype/*.ttf /usr/share/fonts/truetype/ + sudo cp -rfv .builddocs-fonts/opentype/*.otf /usr/share/fonts/opentype/ sudo fc-cache -f -v - name: Build Documentation (${{ matrix.docs-output }}) From 09fb88428693283e0317789d1fe05ae870ea18f2 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 31 Jan 2024 13:21:29 +0000 Subject: [PATCH 9/9] Build documentation when `.github/workflows/build-docs.yml` changes --- .github/workflows/ci.yml | 1 + .github/workflows/nightly.yml | 1 + .github/workflows/scheduled.yml | 1 + .github/workflows/staging.yml | 1 + .github/workflows/templates/layout.yml.jinja | 1 + 5 files changed, 5 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 16573bf856b..b9b53bfa893 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,6 +95,7 @@ jobs: docs: - added|modified: - doc/** + - .github/workflows/build-docs.yml - *doc_requirements workflows: - added|modified: diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 06159dd6f90..bed81903da4 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -139,6 +139,7 @@ jobs: docs: - added|modified: - doc/** + - .github/workflows/build-docs.yml - *doc_requirements workflows: - added|modified: diff --git a/.github/workflows/scheduled.yml b/.github/workflows/scheduled.yml index bd1a41d4fb9..53bfa2db6ca 100644 --- a/.github/workflows/scheduled.yml +++ b/.github/workflows/scheduled.yml @@ -129,6 +129,7 @@ jobs: docs: - added|modified: - doc/** + - .github/workflows/build-docs.yml - *doc_requirements workflows: - added|modified: diff --git a/.github/workflows/staging.yml b/.github/workflows/staging.yml index fa11eb82402..e4280dea294 100644 --- a/.github/workflows/staging.yml +++ b/.github/workflows/staging.yml @@ -125,6 +125,7 @@ jobs: docs: - added|modified: - doc/** + - .github/workflows/build-docs.yml - *doc_requirements workflows: - added|modified: diff --git a/.github/workflows/templates/layout.yml.jinja b/.github/workflows/templates/layout.yml.jinja index 4b146ff3a5f..1c4b54d671b 100644 --- a/.github/workflows/templates/layout.yml.jinja +++ b/.github/workflows/templates/layout.yml.jinja @@ -143,6 +143,7 @@ jobs: docs: - added|modified: - doc/** + - .github/workflows/build-docs.yml - *doc_requirements workflows: - added|modified: