Add decompress module #9175

shamilovstas · 2024-11-22T18:19:01Z

SUMMARY

Adds decompress module (see feature request at #1042)

Fixes #1042

ISSUE TYPE

New Module/Plugin Pull Request

COMPONENT NAME

decompress

ADDITIONAL INFORMATION

adds integration test for gz decompressing

…the module returns error

…tests related to the module core functionality run as parametrized, tests for idempotency and check mode run only for one format)

…ecompression

…rom the src filename

…Adds copyright header to test files

felixfontein

Thanks for your contribution! I've added some first comments.

plugins/modules/decompress.py

felixfontein · 2024-11-23T14:09:29Z

plugins/modules/decompress.py

+
+    def configure(self):
+        if not os.path.exists(self.b_src):
+            self.module.fail_json(msg="Path does not exist: '%s'" % self.b_src)


In case remove=true and the destination path exists, but the source path does not exist, the module should not fail, but simply exit with changed=false. Otherwise the module isn't idempotent.

plugins/modules/decompress.py

felixfontein · 2024-11-23T14:12:12Z

plugins/modules/decompress.py

+        else:
+            self.dest = dest
+        self.b_dest = to_native(self.dest, errors='surrogate_or_strict')
+        self.b_src = to_native(self.src, errors='surrogate_or_strict')


I would expect that b_ indicates bytes, and not native. Why not use byte strings? That's usually the better choice for filesystem paths.

plugins/modules/decompress.py

tests/integration/targets/decompress/files/file.txt

Co-authored-by: Felix Fontein <[email protected]>

…existing dest and remove=true; fixes the issue and adds test)

russoz

Hi @shamilovstas

Thanks for contributing!

Got a few comments, but overall it looks good.

russoz · 2024-11-24T20:45:47Z

plugins/modules/decompress.py

+LZMA_IMP_ERR = None
+if six.PY3:
+    try:
+        import lzma
+
+        HAS_LZMA = True
+    except ImportError:
+        LZMA_IMP_ERR = format_exc()
+        HAS_LZMA = False
+else:
+    try:
+        from backports import lzma
+
+        HAS_LZMA = True
+    except ImportError:
+        LZMA_IMP_ERR = format_exc()
+        HAS_LZMA = False


You might want to consider using module_utils.deps, see https://docs.ansible.com/ansible/latest/collections/community/general/docsite/guide_deps.html for guidance on how to use it.

russoz · 2024-11-24T20:48:17Z

plugins/modules/decompress.py

+            shutil.copyfileobj(src_file, dest_file)
+
+
+class Decompress(object):


You might want to consider using ModuleHelper. See more in https://docs.ansible.com/ansible/latest/collections/community/general/docsite/guide_modulehelper.html

russoz · 2024-11-24T20:58:43Z

plugins/modules/decompress.py

+        self.fmt = self.module.params['format']
+        if self.fmt not in self.handlers:
+            self.module.fail_json(msg="Could not decompress '%s' format" % self.fmt)


The format is controlled by the module parameter, so it is always supposed to have a handler. This code is redundant.

russoz

LGTM

felixfontein · 2024-11-28T19:59:39Z

plugins/modules/decompress.py

+        if not os.path.exists(b_src):
+            msg = "Path does not exist: '%s'" % b_src
+            if self.vars.remove and os.path.exists(b_dest):
+                self.warn(msg)


Why warn in this case? If you really think a warning would be useful in some cases, this should be made configurable, with the default not having a warning.

The idea was that it'd be helpful to let the user know that the file they are trying to decompress does not exist. IMO having a src non-existent is not something that should happen regularly. Initially I made it so that in case of non-existing src the module would fail, but changed it after you commenting that it'd break idempodency. Since now the module finished successfully in this case, I thought that at least the user would be warned about this situation.

This is not something I'm going to insist on, so I'll just delete the warn

A warning is something that should only be used for very important events. For this case a (new) return value is better suited that can be queried by the user if they're interested in this case.

tests/integration/targets/decompress/tasks/core.yml

tests/integration/targets/decompress/tasks/main.yml

shamilovstas added 20 commits November 5, 2024 18:21

adds simple implementation of decompress module

29705f0

adds simple test, fixes src and dest arg types

ecac887

minor refactoring

0473131

adds support for common file operations

f9ff4cd

adds integration test for gz decompressing

makes tests parametrized to test all supported compression formats

b17d344

checks that target file exists

a2c85c6

writes to decompressed file now uses atomic_move

a578fba

adds idempotency for decompression

9580084

refactoring, removed classes

8db0256

adds support for check mode

21f301d

adds check for destination file. If it exists and it is a directory, …

a5bfe90

…the module returns error

refactoring, moves code to a class. Also, simplifies tests (now only …

36bd421

…tests related to the module core functionality run as parametrized, tests for idempotency and check mode run only for one format)

adds 'remove' parameter that deletes original compressed file after d…

c7b6e48

…ecompression

adds documentation

c5a2214

fixes bug with 'remove' parameter in check mode

4816b8f

makes dest argument not required. Dest filename now can be produced f…

203312b

…rom the src filename

adds dest to output

89cd527

updates the documentation, adds "RETURN" block

3e4a174

fixes test

ee9f58c

adds support for python2

c6e7961

ansibullbot added integration tests/integration module module plugins plugin (any type) tests tests labels Nov 22, 2024

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 22, 2024

shamilovstas added 2 commits November 22, 2024 20:45

removes some of the test files that can be generated during testing. …

eba3564

…Adds copyright header to test files

adds maintainer

7774900

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Nov 22, 2024

felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Nov 22, 2024

felixfontein reviewed Nov 23, 2024

View reviewed changes

felixfontein mentioned this pull request Nov 23, 2024

Add uncompress module #1042

Open

ansibullbot added the has_issue label Nov 23, 2024

shamilovstas and others added 2 commits November 23, 2024 21:19

apply minor suggestions from code review

9e34fb1

Co-authored-by: Felix Fontein <[email protected]>

fixes code review comments (idempotency issue with non existing src, …

ef683c1

…existing dest and remove=true; fixes the issue and adds test)

russoz reviewed Nov 24, 2024

View reviewed changes

shamilovstas added 6 commits November 25, 2024 17:31

refactors the module to use ModuleHelper

85a0765

refactors lzma dependency manual check to use 'deps.validate'

52b3f57

minor fix

51b31ee

removes registered handlers check

cbcd66c

minor refactoring

3fbcbd4

adds aliases

8210a7c

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 25, 2024

shamilovstas added 8 commits November 25, 2024 23:27

changes setup for tests

e5917cc

tests: ignores macos and fixes tests for FreeBSD

e2c94ab

tests: reverts ignore for macos and fixes issue with centos7

d2d9159

tests: adds liblzma dependency for python2

28752e3

tests: adds backports.lzma

ae8db2b

fixes bz2 decompression for python2

e68936b

tests: install xz for osx

893b92a

tests: install xz for osx (2)

bab6620

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 26, 2024

shamilovstas requested review from russoz and felixfontein November 26, 2024 20:05

russoz approved these changes Nov 27, 2024

View reviewed changes

felixfontein reviewed Nov 28, 2024

View reviewed changes

fixes code review comments

6e7fed0

shamilovstas requested a review from felixfontein November 28, 2024 21:15

felixfontein approved these changes Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decompress module #9175

Add decompress module #9175

shamilovstas commented Nov 22, 2024

This comment was marked as outdated.

felixfontein left a comment

felixfontein Nov 23, 2024

shamilovstas Nov 23, 2024

felixfontein Nov 23, 2024

shamilovstas Nov 23, 2024

russoz left a comment

russoz Nov 24, 2024

russoz Nov 24, 2024

russoz Nov 24, 2024

russoz left a comment

felixfontein Nov 28, 2024

shamilovstas Nov 28, 2024

shamilovstas Nov 28, 2024

felixfontein Nov 29, 2024

		shutil.copyfileobj(src_file, dest_file)


		class Decompress(object):

Add decompress module #9175

Are you sure you want to change the base?

Add decompress module #9175

Conversation

shamilovstas commented Nov 22, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment