Conversation
|
cc @sheagcraig @garethgreenaway members of the macOS working group. |
| ) | ||
|
|
||
| contents += "\n" | ||
| if isinstance(contents, str): |
There was a problem hiding this comment.
the previous implementation would attempt to add a new line to the end of a binary plist. which would cause sadness.
There was a problem hiding this comment.
My guess is that was written using Py2, where binary == str == text. In Py3, binary == bytes and text == str. Definitely a good move here - unless we should have an
else:
contents += b"\n"
There was a problem hiding this comment.
@waynew Would it make sense to check to see if a new line already exists? I know plistlib automatically adds a newline.
There was a problem hiding this comment.
I'm not familiar with other tools out there, but I would assume we should do whatever the "correct" thing is.
There was a problem hiding this comment.
I've updated with the recommendation as well as skipping new line concatenation on plists as this will create an invalid plist file.
salt/states/file.py
Outdated
| } | ||
|
|
||
| with salt.utils.files.fopen(name, "r") as fhr: | ||
| with salt.utils.files.fopen(name, "rb") as fhr: |
There was a problem hiding this comment.
trying to read a binary plist without the "b" would cause an exception. Also appears all the other implementations of salt.utils.files.fopen in this file use "rb" as arguments.
There was a problem hiding this comment.
I moved this back to its previous behavior and only add a "b" if the formatter is a plist. this change was causing the integration.states.test_file.FileTest.test_serializer_deserializer_opts test to fail.
salt/serializers/plist.py
Outdated
| """ | ||
| salt.serializers.plist | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| .. versionadded:: TODO |
There was a problem hiding this comment.
Would this be 3001?
There was a problem hiding this comment.
Should be, assuming all is 🥇
|
@wes and I wargamed this one and I think it looks ✨🤘🏻✨ |
…catenation, include file state integration tests for plist
…dd_plist_serializer
waynew
left a comment
There was a problem hiding this comment.
The module at salt/serializers/plist.py does not have a sphinx stub at doc/ref/serializers/all/salt.serializers.plist.rst
Looks like you're missing some docs. I think you can autogen - just take a look at the other docs around there.
…ructions for making a binary plist.
Should be added. I think I did that right :) |
…dd_plist_serializer
This reverts commit 7512218.
|
wooo, thanks all! |
What does this PR do?
Adds a plist serializer, allowing you to manage plist files with
file.serialize.Previous Behavior
Before the only way to manage and manipulate plists, a common file type on macOS, was to use
file.managewith a source file.New Behavior
you can now use a
formatterofplistin afile.serializestate to manage xml and binary plist files.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes