[release_2.1] Keep worker --private-data-dir when --delete not given (#…

…887) (#919)

[release_2.1] Keep worker --private-data-dir when --delete not given (#887)

Backport of #887 for Ansible Runner 2.1.

Reviewed-by: David Shrewsbury <None>
Reviewed-by: None <None>
Reviewed-by: Alan Rominger <arominge@redhat.com>
Reviewed-by: Bianca Henderson <beeankha@gmail.com>

ansible_runner/__main__.py

@@ -793,18 +793,15 @@ def main(sys_args=None):
             print(safe_dump(info, default_flow_style=True))
             parser.exit(0)
 
-        cleanup_data_dir = True
-        if vargs.get('private_data_dir'):
-            if os.path.exists(vargs.get('private_data_dir')):
-                if vargs.get('delete_directory', False):
-                    shutil.rmtree(vargs['private_data_dir'])
-                else:
-                    cleanup_data_dir = False
-        else:
+        private_data_dir = vargs.get('private_data_dir')
+        delete_directory = vargs.get('delete_directory', False)
+        if private_data_dir and delete_directory:
+            shutil.rmtree(private_data_dir, ignore_errors=True)
+            register_for_cleanup(private_data_dir)
+        elif private_data_dir is None:
             temp_private_dir = tempfile.mkdtemp()
             vargs['private_data_dir'] = temp_private_dir
-        if cleanup_data_dir:
-            register_for_cleanup(vargs['private_data_dir'])
+            register_for_cleanup(temp_private_dir)
 
     if vargs.get('command') == 'process':
         # the process command is the final destination of artifacts, user expects private_data_dir to not be cleaned up

docs/remote_jobs.rst

@@ -38,8 +38,8 @@ private data directory or artifacts, because these are how the user interacts wi
 
 When running `ansible-runner worker`, if no `--private-data-dir` is given,
 it will extract the contents to a temporary directory which is deleted at the end of execution.
-You can use the `--delete` flag in conjunction with `--private-data-dir` to assure that
-the provided directory is deleted at the end of execution.
+If the ``--private-data-dir`` option is given, then the directory will persist after the run finishes
+unless the ``--delete`` flag is also set. In that case, the private data directory will be deleted before execution if it exists and also removed after execution.
 
 The following command offers out-of-band cleanup.
 

test/integration/test_transmit_worker_process.py

@@ -184,27 +184,67 @@ def transmit_stream(project_fixtures, tmp_path):
         return outgoing_buffer
 
 
-@pytest.mark.parametrize('delete', [False, True])
-def test_worker_preserve_or_delete_dir(tmp_path, cli, transmit_stream, delete):
+def test_worker_without_delete_no_dir(tmp_path, cli, transmit_stream):
+    worker_dir = tmp_path / 'for_worker'
+
+    with open(transmit_stream, 'rb') as stream:
+        worker_args = ['worker', '--private-data-dir', str(worker_dir)]
+        r = cli(worker_args, stdin=stream)
+
+    assert '{"eof": true}' in r.stdout
+    assert worker_dir.joinpath('project', 'debug.yml').exists()
+
+
+def test_worker_without_delete_dir_exists(tmp_path, cli, transmit_stream):
     worker_dir = tmp_path / 'for_worker'
     worker_dir.mkdir()
 
     test_file_path = worker_dir / 'test_file.txt'
-    with test_file_path.open('w') as f:
-        f.write('foobar')
+    test_file_path.write_text('data\n')
 
-    with open(transmit_stream, 'rb') as f:
+    with open(transmit_stream, 'rb') as stream:
         worker_args = ['worker', '--private-data-dir', str(worker_dir)]
-        if delete is True:
-            worker_args.append('--delete')
+        r = cli(worker_args, stdin=stream)
+
+    assert '{"eof": true}' in r.stdout
+    assert worker_dir.joinpath('project', 'debug.yml').exists()
+    assert test_file_path.exists()
+
+
+def test_worker_delete_no_dir(tmp_path, cli, transmit_stream):
+    """
+    Case where non-existing --delete is provided to worker command
+    it should always delete everything both before and after the run
+    """
+    worker_dir = tmp_path / 'for_worker'
+
+    with open(transmit_stream, 'rb') as f:
+        worker_args = ['worker', '--private-data-dir', str(worker_dir), '--delete']
         r = cli(worker_args, stdin=f)
 
     assert '{"eof": true}' in r.stdout
-    for test_path in (test_file_path, worker_dir / 'project' / 'debug.yml'):
-        if delete:
-            assert not test_path.exists()
-        else:
-            assert test_path.exists()
+    assert not worker_dir.exists()
+    assert not worker_dir.joinpath('project', 'debug.yml').exists()
+
+
+def test_worker_delete_dir_exists(tmp_path, cli, transmit_stream):
+    """
+    Case where non-existing --delete is provided to worker command
+    it should always delete everything both before and after the run
+    """
+    worker_dir = tmp_path / 'for_worker'
+    worker_dir.mkdir()
+
+    test_file = worker_dir / 'test_file.txt'
+    test_file.write_text('data\n')
+
+    with open(transmit_stream, 'rb') as f:
+        worker_args = ['worker', '--private-data-dir', str(worker_dir), '--delete']
+        r = cli(worker_args, stdin=f)
+
+    assert '{"eof": true}' in r.stdout
+    assert not worker_dir.exists()
+    assert not worker_dir.joinpath('project', 'debug.yml').exists()
 
 
 def test_missing_private_dir_transmit(tmpdir):

test/unit/__main__/main/test_worker.py

@@ -0,0 +1,45 @@
+import ansible_runner.__main__ as ansible_runner__main__
+import pytest
+
+
+def test_worker_delete(mocker):
+    mock_output = mocker.patch.object(ansible_runner__main__, 'output')
+    mock_output.configure.side_effect = AttributeError('Raised intentionally')
+
+    mock_register_for_cleanup = mocker.patch.object(ansible_runner__main__, 'register_for_cleanup')
+    mock_rmtree = mocker.patch.object(ansible_runner__main__.shutil, 'rmtree')
+    mock_mkdtemp = mocker.patch.object(ansible_runner__main__.tempfile, 'mkdtemp', return_value='some_tmp_dir')
+
+    sys_args = [
+        'worker',
+        '--delete',
+    ]
+
+    with pytest.raises(AttributeError, match='Raised intentionally'):
+        ansible_runner__main__.main(sys_args)
+
+    mock_rmtree.assert_not_called()
+    mock_register_for_cleanup.assert_called_once_with('some_tmp_dir')
+    mock_mkdtemp.assert_called_once()
+
+
+def test_worker_delete_private_data_dir(mocker, tmp_path):
+    mock_output = mocker.patch.object(ansible_runner__main__, 'output')
+    mock_output.configure.side_effect = AttributeError('Raised intentionally')
+
+    mock_register_for_cleanup = mocker.patch.object(ansible_runner__main__, 'register_for_cleanup')
+    mock_rmtree = mocker.patch.object(ansible_runner__main__.shutil, 'rmtree')
+    mock_mkdtemp = mocker.patch.object(ansible_runner__main__.tempfile, 'mkdtemp', return_value='some_tmp_dir')
+
+    sys_args = [
+        'worker',
+        '--private-data-dir', str(tmp_path),
+        '--delete',
+    ]
+
+    with pytest.raises(AttributeError, match='Raised intentionally'):
+        ansible_runner__main__.main(sys_args)
+
+    mock_rmtree.assert_called_once_with(str(tmp_path), ignore_errors=True)
+    mock_register_for_cleanup.assert_called_once_with(str(tmp_path))
+    mock_mkdtemp.assert_not_called()