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

Update tojson to allow arbitrary args #56022

Merged
merged 9 commits into from Mar 5, 2021

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Jan 29, 2020

What does this PR do?

The builtin Jinja tojson filter only supports indent as an arg that can be passed to the underlying json dump function. This doesn't allow all the functionality people might want.

This updates the tojson filter to allow arbitrary keyword arguments to be passed along to the underlying JSON library.

What issues does this PR fix or reference?

#55911 and closes #56012

Previous Behavior

It was impossible to get unsorted JSON, or use other functionality.

New Behavior

It's possible to specify sorted or unsorted JSON, as well as pass arbitrary arguments to the underlying JSON dump function.

Tests written?

Yes

Commits signed with GPG?

Yes

@waynew waynew requested a review from a team as a code owner January 29, 2020 23:56
@ghost ghost requested a review from dwoz January 29, 2020 23:56
@waynew waynew added the ZRelease-Sodium retired label label Jan 29, 2020
cmcmarrow
cmcmarrow previously approved these changes Mar 23, 2020
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 6, 2020
@DmitryKuzmenko DmitryKuzmenko dismissed stale reviews from cmcmarrow and themself via f5bfa16 April 6, 2020 18:57
@DmitryKuzmenko DmitryKuzmenko self-assigned this Apr 6, 2020
@waynew waynew force-pushed the 55911-to-json-sorted-jinja branch 2 times, most recently from f2a0391 to 09b21e2 Compare April 28, 2020 17:29
@DmitryKuzmenko
Copy link
Contributor

@waynew it looks failures are related.

@waynew waynew force-pushed the 55911-to-json-sorted-jinja branch from fb41c6c to da7d833 Compare May 12, 2020 17:41
@sagetherage sagetherage added this to In progress in Sodium via automation May 12, 2020
@sagetherage sagetherage removed the ZRelease-Sodium retired label label May 26, 2020
@sagetherage sagetherage removed this from In progress in Sodium May 26, 2020
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label May 26, 2020
@sagetherage sagetherage assigned waynew and unassigned DmitryKuzmenko Jun 1, 2020
@sagetherage sagetherage removed the Magnesium Mg release after Na prior to Al label Jun 1, 2020
@waynew waynew force-pushed the 55911-to-json-sorted-jinja branch from 90da99e to 644b723 Compare June 2, 2020 20:34
@sagetherage sagetherage added Magnesium Mg release after Na prior to Al Bug broken, incorrect, or confusing behavior severity-high 2nd top severity, seen by most users, causes major problems labels Sep 16, 2020
dwoz
dwoz previously approved these changes Sep 29, 2020
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Sep 30, 2020
@waynew
Copy link
Contributor Author

waynew commented Sep 30, 2020

rebased + py2 drop precommit. Should be good now (or in a couple hours when all tests are done)

@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 5, 2020

ping @waynew theres some failing tests here

@sagetherage sagetherage added Aluminium Release Post Mg and Pre Si and removed Magnesium Mg release after Na prior to Al labels Oct 8, 2020
@sagetherage sagetherage added this to In progress in Aluminium Oct 20, 2020
@sagetherage
Copy link
Contributor

moving this to another release

@sagetherage sagetherage added the point-release minor release label Mar 3, 2021
@waynew waynew force-pushed the 55911-to-json-sorted-jinja branch from 2b8c235 to 21d32c2 Compare March 3, 2021 23:39
I goofed and was not sorting in one test - that's fixed. The other test
was failing because earlier versions of Python don't remember dict
insertion/creation order. Without sorting the output order was
accidental. On certain platforms they were accidental in a different
order than expected, so this caused a problem.

Changing to OrderedDict allows us to specify the order, and this should
work on those older versions of Python.
@waynew
Copy link
Contributor Author

waynew commented Mar 4, 2021

@sagetherage hopefully this should pass the broken tests -- I fell victim to one of the classic blunders - no, not "Never get involved in a land war in Asia,"... and no, not "Never go up against a Sicilian when death is on the line" This one was, "Earlier versions of Python are arbitrary with their dictionary ordering no matter how lucky you think you are."

Anyway, this should be better now, please let me know if it isn't!

@Ch3LL Ch3LL merged commit 3817bfb into saltstack:master Mar 5, 2021
30 checks passed
@myii
Copy link
Contributor

myii commented Aug 4, 2021

@waynew Note that the tojson section in https://github.com/saltstack/salt/blob/master/doc/topics/jinja/index.rst also needs to be updated.

.. jinja_ref:: tojson

``tojson``
----------

.. versionadded:: 2018.3.3,2019.2.0

Dumps a data structure to JSON.

This filter was added to provide this functionality to hosts which have a
Jinja release older than version 2.9 installed. If Jinja 2.9 or newer is
installed, then the upstream version of the filter will be used. See the
`upstream docs`__ for more information.

I.e. This part:

If Jinja 2.9 or newer is installed, then the upstream version of the filter will be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si Bug broken, incorrect, or confusing behavior point-release minor release severity-high 2nd top severity, seen by most users, causes major problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to sort keys (or pass extra args) on tojson in Python3/json_encode_dict doesn't produce valid JSON
9 participants