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

Address file.managed with contents_newline flag #54929

Merged
merged 11 commits into from
Dec 9, 2019
13 changes: 9 additions & 4 deletions salt/states/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -2311,8 +2311,10 @@ def managed(name,

If ``True``, files managed using ``contents``, ``contents_pillar``, or
``contents_grains`` will have a newline added to the end of the file if
one is not present. Setting this option to ``False`` will omit this
final newline.
one is not present. Setting this option to ``False`` will ensure the
final line, or entry, does not contain a new line. If the last line, or
entry in the file does contain a new line already, this option will not
remove it.

contents_delimiter
.. versionadded:: 2015.8.4
Expand Down Expand Up @@ -2635,8 +2637,11 @@ def managed(name,
for part in validated_contents:
for line in part.splitlines():
contents += line.rstrip('\n').rstrip('\r') + os.linesep
if contents_newline and not contents.endswith(os.linesep):
contents += os.linesep
if not contents_newline:
# If contents newline is set to False, strip out the newline
# character and carriage return character
contents = contents.rstrip('\n').rstrip('\r')

except UnicodeDecodeError:
# Either something terrible happened, or we have binary data.
if template:
Expand Down
62 changes: 62 additions & 0 deletions tests/integration/states/test_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,68 @@ def test_managed_contents(self):
if os.path.exists(managed_files[typ]):
os.remove(managed_files[typ])

def test_managed_contents_with_contents_newline(self):
'''
test file.managed with contents by using the default content_newline
flag.
'''
contents = 'test_managed_contents_with_newline_one'
name = os.path.join(TMP, 'foo')

# Create a file named foo with contents as above but with a \n at EOF
self.run_state('file.managed', name=name, contents=contents,
contents_newline=True)
with salt.utils.files.fopen(name, 'r') as fp_:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be appropriate to open this in rb mode instead? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you aren't wrong! I must have forgotten the b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no I believe it should remain as r. I thought about this and the PR was incorrectly named (definitely my fault) but opening the file in binary mode and then reading it will produce different results based on the platform, i.e. Window's will read the contents differently than a linux platform. So I decided to just open the file in r as it's more in-line with the change in logic itself. Does that make sense?

last_line = fp_.read()
self.assertEqual((contents + os.linesep), last_line)

def test_managed_contents_with_contents_newline_false(self):
'''
test file.managed with contents by using the non default content_newline
flag.
'''
contents = 'test_managed_contents_with_newline_one'
name = os.path.join(TMP, 'bar')

# Create a file named foo with contents as above but with a \n at EOF
self.run_state('file.managed', name=name, contents=contents,
contents_newline=False)
with salt.utils.files.fopen(name, 'r') as fp_:
last_line = fp_.read()
self.assertEqual(contents, last_line)

def test_managed_multiline_contents_with_contents_newline(self):
'''
test file.managed with contents by using the non default content_newline
flag.
'''
contents = ('this is a cookie{}this is another cookie'.
format(os.linesep))
name = os.path.join(TMP, 'bar')

# Create a file named foo with contents as above but with a \n at EOF
self.run_state('file.managed', name=name, contents=contents,
contents_newline=True)
with salt.utils.files.fopen(name, 'r') as fp_:
last_line = fp_.read()
self.assertEqual((contents + os.linesep), last_line)

def test_managed_multiline_contents_with_contents_newline_false(self):
'''
test file.managed with contents by using the non default content_newline
flag.
'''
contents = ('this is a cookie{}this is another cookie'.
format(os.linesep))
name = os.path.join(TMP, 'bar')

# Create a file named foo with contents as above but with a \n at EOF
self.run_state('file.managed', name=name, contents=contents,
contents_newline=False)
with salt.utils.files.fopen(name, 'r') as fp_:
last_line = fp_.read()
self.assertEqual(contents, last_line)

@skip_if_not_root
@skipIf(IS_WINDOWS, 'Windows does not support "mode" kwarg. Skipping.')
@skipIf(not salt.utils.path.which('visudo'), 'sudo is missing')
Expand Down