All of Salt's loaders now accept `loaded_base_name` & Only functions defined on the modules being loaded will be added to the lazy loader #62189

s0undt3ch · 2022-06-17T12:27:08Z

All of Salt's loaders now accept loaded_base_name as a keyword argument, allowing different namespacing the loaded modules.
Additionally, only functions defined on the modules being loaded will be added to the lazy loader, functions imported from other modules, unless they are properly namespaced, are not included.

s0undt3ch · 2022-06-17T14:12:12Z

re-run full all

waynew

Just a couple of questions here - The one yaml -> yamldumper is probably a blocker. I think the others are more just questions on my part. I'm not sure if there's a rush to get this in -- if so it's probably fine to merge before I get back & can re-review. Otherwise, I would like to bang on it a little more, just to make sure I really understand what the impact is for the LazyLoader 🤔 (I mean, I don't see anything else that's giving me cause for concern - just the fact that we're making the change for the Loader that affects all of salt)

waynew · 2022-06-17T21:28:55Z

salt/modules/boto_cloudfront.py

@@ -263,8 +263,8 @@ def export_distributions(region=None, key=None, keyid=None, profile=None):
        # as opposed to being called from execution or state modules
        log.trace("Boto client error: {}", exc)

-    dumper = __utils__["yaml.get_dumper"]("IndentedSafeOrderedDumper")
-    return __utils__["yaml.dump"](
+    dumper = __utils__["yamldumper.get_dumper"]("IndentedSafeOrderedDumper")


question do all these need to be changed?

✦2 ❯ ag '"yaml\.' salt salt/modules/boto_iam.py 1682: return __utils__["yaml.safe_dump"](results, default_flow_style=False, indent=2) 1728: return __utils__["yaml.safe_dump"](results, default_flow_style=False, indent=2)

I'm assuming that this only applies for __utils__ and not __serializers__ - I just went and found a couple of those, specifically in salt/states/file.py - it looks for serialzer_name in serializers where yaml.serialize is what's called, which doesn't look like the yamldumper 🤔

The yamldumper only applies to the utils dunder.
They were never using our yamldumper utils module, they were using the actual yaml package.

waynew · 2022-06-17T21:39:18Z

tests/pytests/unit/states/test_boto_cloudfront.py

@@ -30,7 +30,7 @@ def configure_loader_modules(setup_vars):
        whitelist=[
            "boto3",
            "dictdiffer",
-            "yaml",
+            "yamldumper",


question if I do git checkout salt/master -- salt/loader/lazy.py then this doesn't fail 🤔 It only fails if I leave lazy as-is and change this back. Why is that?

Because now the loader, correctly, only adds functions defined in the module, not functions imported into the module, unless these are namespaced into the module scope.

waynew · 2022-06-17T21:39:18Z

tests/pytests/unit/states/test_boto_cloudfront.py

@@ -30,7 +30,7 @@ def configure_loader_modules(setup_vars):
        whitelist=[
            "boto3",
            "dictdiffer",
-            "yaml",
+            "yamldumper",


question if I do git checkout salt/master -- salt/loader/lazy.py then this doesn't fail 🤔 It only fails if I leave lazy as-is and change this back. Why is that?

Same answer.

s0undt3ch · 2022-06-17T21:50:18Z

Just a couple of questions here - The one yaml -> yamldumper is probably a blocker. I think the others are more just questions on my part. I'm not sure if there's a rush to get this in -- if so it's probably fine to merge before I get back & can re-review. Otherwise, I would like to bang on it a little more, just to make sure I really understand what the impact is for the LazyLoader 🤔 (I mean, I don't see anything else that's giving me cause for concern - just the fact that we're making the change for the Loader that affects all of salt)

The rush is to get it into 3005.
Until we freeze, I'm good to leave this open for further review.

s0undt3ch · 2022-06-17T21:54:49Z

To complement, the lazy loader, prior to these changes loaded every function or partial it could get its hands on, namely due to the loader doing a dir(<module>).

This is the wrong behavior because the loader should only care about functions defined on the module being loaded.

If we did from json import dumps into any module which was loaded by the loader, then, dumps would be available on the dunder.

On some rare occasions, that is the intended behavior, and thats why the namespace_function function was created.

s0undt3ch · 2022-06-17T21:56:00Z

However, we were failing to enforce these constraints.

s0undt3ch · 2022-06-17T22:12:09Z

This is why we should never allowed __utils__ to be a thing.
The __utils__['yaml.*'] worked because of these imports.

Which goes against how the LazyLoader was intended to work.
It relied on a side effect of not applying the original intended constraints.

s0undt3ch · 2022-06-17T22:17:02Z

For reference, https://github.com/saltstack/salt/blob/v3004.1/salt/utils/functools.py#L15

Fixes saltstack#62186 Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

Fixes saltstack#62190 Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

github-actions · 2022-06-18T06:09:05Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.06s
- exit code: 1
/home/runner/.cache/pre-commit/repoavddhtkl/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:30: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'delete_alarm' on 'salt/modules/boto_cloudwatch.py' does not have a 'CLI Example:' in its docstring

Found 1 errors

Thanks again!

s0undt3ch · 2022-06-18T06:53:40Z

The __utils__ relies on so much of the bad behavior that I decided to leave it as it was until we completely get rid of __utils__.
Too many things would break.

…funcs in ``__utils__`` !@#$%@!#@$%!$@#$T Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

twangboy

Shouldn't we add some documentation about the loaded_base_name option in the salt/loader/__init__.py file? We mention it in the changelog, but I think it should be documented in the __init__.py where that is being added as an additional option.

s0undt3ch · 2022-06-21T16:56:27Z

Shouldn't we add some documentation about the loaded_base_name option in the salt/loader/__init__.py file? We mention it in the changelog, but I think it should be documented in the __init__.py where that is being added as an additional option.

Can I do it on a follow up PR if @waynew approves this one? I'll do it right after merge.

Ch3LL · 2022-06-29T13:40:17Z

@waynew when you get time to re-review this, Pedro will do a follow up PR if you see any more issues.

s0undt3ch requested a review from a team as a code owner June 17, 2022 12:27

s0undt3ch requested review from waynew and removed request for a team June 17, 2022 12:27

s0undt3ch force-pushed the issues/62186-loaded-base-name branch from 123b47e to 0be65f0 Compare June 17, 2022 12:32

Ch3LL previously approved these changes Jun 17, 2022

View reviewed changes

s0undt3ch dismissed Ch3LL’s stale review via 7521f55 June 17, 2022 14:10

saltstack deleted a comment from github-actions bot Jun 17, 2022

s0undt3ch force-pushed the issues/62186-loaded-base-name branch from 5e6bb1e to 94e39ea Compare June 17, 2022 19:07

saltstack deleted a comment from github-actions bot Jun 17, 2022

waynew suggested changes Jun 17, 2022

View reviewed changes

s0undt3ch added 2 commits June 18, 2022 07:05

Allow passing loaded_base_name to each of the loaders salt supports.

3ece1a6

Fixes saltstack#62186 Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

Stop loading imported functions.

85d16f0

Fixes saltstack#62190 Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

s0undt3ch force-pushed the issues/62186-loaded-base-name branch from 94e39ea to 41102c9 Compare June 18, 2022 06:07

s0undt3ch force-pushed the issues/62186-loaded-base-name branch from 41102c9 to 6a7e599 Compare June 18, 2022 06:52

As much as I hate this, we can't, currently, only include namespaced …

ab42a89

…funcs in ``__utils__`` !@#$%@!#@$%!$@#$T Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

s0undt3ch force-pushed the issues/62186-loaded-base-name branch from 6a7e599 to ab42a89 Compare June 18, 2022 07:07

s0undt3ch requested review from waynew and Ch3LL June 21, 2022 09:32

twangboy requested changes Jun 21, 2022

View reviewed changes

twangboy approved these changes Jun 21, 2022

View reviewed changes

s0undt3ch added the Phosphorus v3005.0 Release code name and version label Jun 23, 2022

s0undt3ch added this to the Phosphorus v3005.0 milestone Jun 23, 2022

Ch3LL merged commit 29d66ec into saltstack:master Jun 29, 2022

OrangeDog mentioned this pull request Aug 30, 2022

[BUG] x509.pem_managed is not available #62551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All of Salt's loaders now accept `loaded_base_name` & Only functions defined on the modules being loaded will be added to the lazy loader #62189

All of Salt's loaders now accept `loaded_base_name` & Only functions defined on the modules being loaded will be added to the lazy loader #62189

s0undt3ch commented Jun 17, 2022 •

edited

Loading

s0undt3ch commented Jun 17, 2022

waynew left a comment •

edited

Loading

waynew Jun 17, 2022

s0undt3ch Jun 17, 2022

waynew Jun 17, 2022

s0undt3ch Jun 17, 2022

waynew Jun 17, 2022

s0undt3ch Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

github-actions bot commented Jun 18, 2022

s0undt3ch commented Jun 18, 2022

twangboy left a comment

s0undt3ch commented Jun 21, 2022

Ch3LL commented Jun 29, 2022

All of Salt's loaders now accept loaded_base_name & Only functions defined on the modules being loaded will be added to the lazy loader #62189

All of Salt's loaders now accept loaded_base_name & Only functions defined on the modules being loaded will be added to the lazy loader #62189

Conversation

s0undt3ch commented Jun 17, 2022 • edited Loading

s0undt3ch commented Jun 17, 2022

waynew left a comment • edited Loading

Choose a reason for hiding this comment

waynew Jun 17, 2022

Choose a reason for hiding this comment

s0undt3ch Jun 17, 2022

Choose a reason for hiding this comment

waynew Jun 17, 2022

Choose a reason for hiding this comment

s0undt3ch Jun 17, 2022

Choose a reason for hiding this comment

waynew Jun 17, 2022

Choose a reason for hiding this comment

s0undt3ch Jun 17, 2022

Choose a reason for hiding this comment

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

s0undt3ch commented Jun 17, 2022

github-actions bot commented Jun 18, 2022

Hi! I'm your friendly PR bot!

s0undt3ch commented Jun 18, 2022

twangboy left a comment

Choose a reason for hiding this comment

s0undt3ch commented Jun 21, 2022

Ch3LL commented Jun 29, 2022

All of Salt's loaders now accept `loaded_base_name` & Only functions defined on the modules being loaded will be added to the lazy loader #62189

All of Salt's loaders now accept `loaded_base_name` & Only functions defined on the modules being loaded will be added to the lazy loader #62189

s0undt3ch commented Jun 17, 2022 •

edited

Loading

waynew left a comment •

edited

Loading