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

Support querying for JSON data in external sql pillar. #59777

Merged
merged 8 commits into from Oct 13, 2021

Conversation

aaannz
Copy link
Contributor

@aaannz aaannz commented Mar 11, 2021

What does this PR do?

Querying database for single column JSON data (like postgres JSONB) returns dict. Dict is not hashable and results in error in sql external pillar. This PR add option as_json which assumes that the query result is dict and merges result with returning pillar dict.

What issues does this PR fix or reference?

Fixes:

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.

@aaannz aaannz force-pushed the sql-json branch 3 times, most recently from ffd96d9 to 5a0554b Compare March 11, 2021 19:15
@aaannz aaannz marked this pull request as ready for review March 11, 2021 22:36
@aaannz aaannz requested a review from a team as a code owner March 11, 2021 22:36
@aaannz aaannz requested review from twangboy and removed request for a team March 11, 2021 22:36
twangboy
twangboy previously approved these changes Mar 12, 2021
@aaannz
Copy link
Contributor Author

aaannz commented Mar 15, 2021

I added changelog entry in format PR#.added as I did not create issue before. Is it ok, or should I create issue and link issue instead?
Is there anything else I should do?

@cbosdo
Copy link
Contributor

cbosdo commented Sep 21, 2021

Implements #60905

@cbosdo
Copy link
Contributor

cbosdo commented Sep 23, 2021

@Ch3LL could you or someone in the core team have a look at this PR?

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbosdo Looks great! Just one comment that needs addressing, then I can go ahead and approve 👍 thanks!

tests/pytests/unit/pillar/test_sql_base.py Show resolved Hide resolved
salt/pillar/sql_base.py Show resolved Hide resolved
Several SQL databases support native JSON storage. When storing
pillars in this way, SQL query result already returns dict and
without the need to have key column.
aaannz and others added 2 commits September 29, 2021 17:03
Use salt.utils.update() to recursively merge the JSON dicts of the
returned SQL queries.
@cbosdo cbosdo force-pushed the sql-json branch 2 times, most recently from f373340 to 46b63e5 Compare September 30, 2021 07:49
@@ -147,6 +174,9 @@

Configuration of the connection depends on the adapter in use.

.. versionadded:: 3005
The *to_json* parameter.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The *to_json* parameter.
The *as_json* parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed now

@Ch3LL Ch3LL requested a review from waynew October 5, 2021 16:44
@Ch3LL Ch3LL added the Phosphorus v3005.0 Release code name and version label Oct 8, 2021
@Ch3LL Ch3LL merged commit acfae70 into saltstack:master Oct 13, 2021
@welcome
Copy link

welcome bot commented Oct 13, 2021

Congratulations on your first PR being merged! 🎉

@aaannz aaannz deleted the sql-json branch October 13, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants