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

file.serialize fails to serialize due to ordered dicts #20647

Closed
ryan-lane opened this issue Feb 12, 2015 · 7 comments · Fixed by #20779
Closed

file.serialize fails to serialize due to ordered dicts #20647

ryan-lane opened this issue Feb 12, 2015 · 7 comments · Fixed by #20779
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix severity-low 4th level, cosemtic problems, work around exists
Milestone

Comments

@ryan-lane
Copy link
Contributor

It's possible file.serialize has been broken since everything has been switched to ordered dicts. It seems to pass data structures without transforming them and ordered dicts aren't serializable.

@rfairburn
Copy link
Contributor

Here is an example of things being broken in salt 2014.7.0: https://bpaste.net/show/8b5d522ca1b3

For reference, this may be specific to the yaml formatter as the json formatter works as expected..

@rfairburn
Copy link
Contributor

It looks like this had been worked around elsewhere previously here: https://github.com/saltstack/salt/blob/2014.7/salt/utils/jinja.py#L37-L43

I would assume this same solution could be used for the formatter.

@jfindlay jfindlay added Bug broken, incorrect, or confusing behavior Medium Change labels Feb 12, 2015
@jfindlay jfindlay added this to the Approved milestone Feb 12, 2015
cachedout pushed a commit to cachedout/salt that referenced this issue Feb 17, 2015
@cachedout
Copy link
Contributor

Hi @ryan-lane and @rfairburn. Please give #20779 a try and let me know how it goes. In my testing it resolves this issue. Thanks!

@cachedout cachedout added the fixed-pls-verify fix is linked, bug author to confirm fix label Feb 17, 2015
@jfindlay jfindlay added severity-low 4th level, cosemtic problems, work around exists and removed Medium Change labels Feb 18, 2015
@rfairburn
Copy link
Contributor

@cachedout, I was able to successfully test this this today. Here is a paste of it working:
https://bpaste.net/show/95ba3d38f4ed

@rallytime
Copy link
Contributor

@rfairburn Thanks for giving that a try and confirming the fix! Once @cachedout's PR gets merged in, I think we should be able to close this one.

@favadi
Copy link

favadi commented Apr 28, 2015

@cachedout @rallytime the bug is existed in 2014.7 branch.Is it a good idea to backport this fix to 2014.7 branch?

@jfindlay
Copy link
Contributor

@favadi, yes, we should backport it, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix severity-low 4th level, cosemtic problems, work around exists
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants