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

Fix use of deserializer_opts in file.serialize #52526

Merged
merged 2 commits into from Apr 17, 2019

Conversation

@lomeroe
Copy link
Contributor

commented Apr 12, 2019

What does this PR do?

Corrects code attempting to pull deserialize_opts using the serializer_name instead of deserializer_name

What issues does this PR fix or reference?

#52525

Previous Behavior

deserializer_opts are packed into the deserializer_options dict using the deserialzier_name as the key. However, when retrieved to pass to the deserializer, the serializer_name is used.

New Behavior

deserializer_name is used to retrieve the options

Tests written?

No

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.

when deserializer_name is used as the key in the deserializer_options
dict.
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

thanks for the PR! Is it possible for you to write a regression test for this change?

@lomeroe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@Ch3LL integration tests updated

Copy link
Contributor

left a comment

@lomeroe It looks like we're removing coverage from the json formatter in your test change. I'm concerned that we'll now catch a bug when using the configparser formatter but miss a bug when using the json formatter. Maybe it's better to make a new test for configparser?

@lomeroe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@dwoz the purpose of that test seemed to only be to validate that the serializer_opts and deserializer_opts being passed in the state were functioning correctly (and being used), which it was not doing. My opinion was that the particular formatter did not make a difference for the test case. Configparser is the easiest to assert, from a data perspective, that the serializer/deserializer options are in effect. For the json formatter, there are no options that can be passed (as a string) to modify the returned data (parameters that can must be passed in as functions) to validate the option's functionality. The json test could be kept, but modified to pass an encoding such as utf-16 or utf-32 which will raise an error and that could be asserted. As it was, passing an encoding of latin-1 is no different than the default, and can't be validated that it was actually passed to the deserializer function (which is why the test wasn't catching that the options weren't even being passed).

In my opinion, I don't see that it gains anything to test options to both formatters, but at the same time have no issue putting the test in there if you want it.

@dwoz
dwoz approved these changes Apr 17, 2019
Copy link
Contributor

left a comment

@lomeroe that makes sense and thank you for the clarification. Approved! :)

@dwoz dwoz merged commit 21d6365 into saltstack:2019.2 Apr 17, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.