Skip to content

Commit

Permalink
[archive] fix add_string()/do_*_sub() regression
Browse files Browse the repository at this point in the history
A change in the handling of add_string() operations in the archive
class causes the Plugin string substitution methods to fail (since
the archive was enforcing a check that the path did not already
exist - for substitutions this is always the case).

Maintain the check for content that is being copied into the
archive anew, but make the add_string() method override this and
disable the existence checks.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
  • Loading branch information
bmr-cymru committed Jul 2, 2018
1 parent be32615 commit b96bdab
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
14 changes: 10 additions & 4 deletions sos/archive.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def dest_path(self, name):
name = name.lstrip(os.sep)
return (os.path.join(self._archive_root, name))

def _check_path(self, src, path_type, dest=None):
def _check_path(self, src, path_type, dest=None, force=False):
"""Check a new destination path in the archive.
Since it is possible for multiple plugins to collect the same
Expand All @@ -185,6 +185,7 @@ def _check_path(self, src, path_type, dest=None):
:param src: the source path to be copied to the archive
:param path_type: the type of object to be copied
:param dest: an optional destination path
:param force: force file creation even if the path exists
:returns: An absolute destination path if the path should be
copied now or `None` otherwise
"""
Expand All @@ -208,6 +209,9 @@ def is_special(mode):
stat.ISSOCK(mode)
])

if force:
return dest

# Check destination path presence and type
if os.path.exists(dest):
# Use lstat: we care about the current object, not the referent.
Expand Down Expand Up @@ -274,9 +278,11 @@ def add_string(self, content, dest):
with self._path_lock:
src = dest

dest = self._check_path(dest, P_FILE)
if not dest:
return
# add_string() is a special case: it must always take precedence
# over any exixting content in the archive, since it is used by
# the Plugin postprocessing hooks to perform regex substitution
# on file content.
dest = self._check_path(dest, P_FILE, force=True)

f = codecs.open(dest, 'w', encoding='utf-8')
if isinstance(content, bytes):
Expand Down
12 changes: 2 additions & 10 deletions tests/archive_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,21 +94,13 @@ def test_get_file(self):
self.assertEquals('this is my content', afp.read())

def test_rewrite_file(self):
"""Test that re-writing a file does not modify the content.
In sos we do not have a use for overwriting archive content
in-place (it is an error if different plugins attempt to
store different content at the same path).
We do not enforce the content check at runtime since it
would be prohibitively costly: instead just verify in the
unit suite that the original content is preserved.
"""Test that re-writing a file with add_string() modifies the content.
"""
self.tf.add_string('this is my content', 'tests/string_test.txt')
self.tf.add_string('this is my new content', 'tests/string_test.txt')

afp = self.tf.open_file('tests/string_test.txt')
self.assertEquals('this is my content', afp.read())
self.assertEquals('this is my new content', afp.read())

def test_make_link(self):
self.tf.add_file('tests/ziptest')
Expand Down

0 comments on commit b96bdab

Please sign in to comment.