-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[master] Fix config.items return value #66364
Conversation
@@ -561,4 +561,4 @@ def items(): | |||
|
|||
salt '*' config.items | |||
""" | |||
return __opts__ | |||
return __opts__.value() |
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 don't think it's always a See below.NamedLoaderContext
.
outdated suggestion
``` ```suggestion try: return __opts__.value() except AttributeError: return __opts__ ```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.
Do you know in what cases it could be not NamedLoaderContext
? As far as I know __opts__
is populated with LazyLoader
and to be fair I'm not sure if it can push something different there.
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.
Tbh now I'm not sure anymore if what I said is correct. I looked up what I remembered when writing the above, but it might have been Salt-SSH-specific only. Sorry for the noise! :)
In general, I think one of the cases where this might happen in a regular module is if the dunder is initialized to a dict during module import in the top-level module code, but that's not the case here (on the contrary).
Edit: So in this module __opts__
is specifically initialized to be a NamedLoaderContext, but once I comment out
Line 40 in aad71fd
__opts__ = __salt_loader__.named_context("__opts__") |
it turns into a dict. Someone with more familiarity with the loader will probably be able to shed some light on this.
@vzhestkov @lkubb This will get fixed by #66649 and has been merged forward. |
@dwoz thanks for pointing, in this case this PR doesn't make sense anymore, closing... |
What does this PR do?
Fix return value of
config.items
to prevent not expected return values.Previous Behavior
Could return the following instead of the proper
dict
:New Behavior
Return the expected
dict
outputMerge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.