Skip to content
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

Fixes bug in modules/pillar.py #58971

Merged
merged 5 commits into from
Oct 22, 2022
Merged

Fixes bug in modules/pillar.py #58971

merged 5 commits into from
Oct 22, 2022

Conversation

tommygrumheden
Copy link
Contributor

What does this PR do?

The method obfuscate needs **kwargs in parameters so its able to use items() fully, as intended.
This fixes that problem.

No tests / changelog / docs supplied in PR.

What issues does this PR fix or reference?

Fixes: No issue created.

Previous Behavior

ERROR executing 'pillar.obfuscate': The following keyword arguments are not valid: saltenv=XXX

New Behavior

saltenv works.

Merge requirements satisfied?

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

Commits signed with GPG?

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.

The method obfuscate needs **kwargs in parameters so its able to use items() fully, as intended.
This fixes that problem.
@tommygrumheden tommygrumheden requested a review from a team as a code owner November 18, 2020 10:16
@tommygrumheden tommygrumheden requested review from twangboy and removed request for a team November 18, 2020 10:16
@welcome
Copy link

welcome bot commented Nov 18, 2020

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at core@saltstack.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

twangboy
twangboy previously approved these changes Oct 20, 2022
@waynew waynew added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 20, 2022
@garethgreenaway
Copy link
Contributor

@tommygrumheden What is the reason for the change blindly accepting kwargs instead of specific keyword arguments?

@tommygrumheden
Copy link
Contributor Author

@tommygrumheden What is the reason for the change blindly accepting kwargs instead of specific keyword arguments?

Hi @garethgreenaway
Sincerely I don't remember. And probably no good reason for using kwargs other that it quickly fixed our problems with saltenv. :)

@garethgreenaway
Copy link
Contributor

@tommygrumheden Thanks. I see why this is needed and it looks like we include it in other functions in the pillar module. I added a quick test to ensure that kwargs such as saltenv are passed along to pillar.items where it's used.

@garethgreenaway garethgreenaway removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 22, 2022
@garethgreenaway garethgreenaway merged commit 609e5e1 into saltstack:master Oct 22, 2022
@welcome
Copy link

welcome bot commented Oct 22, 2022

Congratulations on your first PR being merged! 🎉

@tommygrumheden
Copy link
Contributor Author

@tommygrumheden Thanks. I see why this is needed and it looks like we include it in other functions in the pillar module. I added a quick test to ensure that kwargs such as saltenv are passed along to pillar.items where it's used.

Thanks for fixing a test! Happy to be contributing :) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants