mirror of
https://github.com/ansible-collections/community.general.git
synced 2026-05-07 13:52:54 +00:00
First bit of fixing temporary to have one source of truth (#35747)
* First bit of fixing temporary to have one source of truth * Fix pep8 * Remove explicit make_tmp_path() in copy The copy action plugin sets TRANSFER_FILES=True so it does not need to set the temporary directory explicitly; the base class's run() method will do that for us. * Fix for calling a module's run when a shell has already created a temp path. * Remember to inform the rest of the world when tempdir is removed * New strategy for how to warn on passing tmp Now we just warn when calling the parent class run() early. If the module does a late call to the parent run() and doesn't make use of the temporary directory, then we don't check for the possibility that the user mistakenly is sending tmp in. If we truly deprecate this (rather than ignoring it forever) then we might want to switch back to checking for someone passing a value in as tmp. * Remove tmp parameter from _execute_module as well * Port all action plugins to not send tmp explicitly This is now handled inside of _execute_module via the _connection._shell.tempdir attribute. Also update warnings and docs to tell people to set the attribute instead of using _execute_module's tmp parameter. * Always set local tempdir variable
This commit is contained in:
Toshio Kuratomi
committed by
Matt Davis
Matt Davis
parent
7225839bef
commit
71f46d69d6
@@ -66,9 +66,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
tasks. Everything else in this base class is a helper method for the
|
||||
action plugin to do that.
|
||||
|
||||
:kwarg tmp: Temporary directory. Sometimes an action plugin sets up
|
||||
a temporary directory and then calls another module. This parameter
|
||||
allows us to reuse the same directory for both.
|
||||
:kwarg tmp: Deprecated parameter. This is no longer used. An action plugin that calls
|
||||
another one and wants to use the same remote tmp for both should set
|
||||
self._connection._shell.tempdir rather than this parameter.
|
||||
:kwarg task_vars: The variables (host vars, group vars, config vars,
|
||||
etc) associated with this task.
|
||||
:returns: dictionary of results from the module
|
||||
@@ -80,6 +80,12 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
|
||||
result = {}
|
||||
|
||||
if tmp is not None:
|
||||
result['warning'] = ['ActionModule.run() no longer honors the tmp parameter. Action'
|
||||
' plugins should set self._connection._shell.tempdir to share'
|
||||
' the tempdir']
|
||||
del tmp
|
||||
|
||||
if self._task.async_val and not self._supports_async:
|
||||
raise AnsibleActionFail('async is not supported for this task.')
|
||||
elif self._play_context.check_mode and not self._supports_check_mode:
|
||||
@@ -87,10 +93,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
elif self._task.async_val and self._play_context.check_mode:
|
||||
raise AnsibleActionFail('check mode and async cannot be used on same task.')
|
||||
|
||||
if not tmp and self._early_needs_tmp_path():
|
||||
if self._connection._shell.tempdir is None and self._early_needs_tmp_path():
|
||||
self._make_tmp_path()
|
||||
else:
|
||||
self._connection._shell.tempdir = tmp
|
||||
|
||||
return result
|
||||
|
||||
@@ -311,6 +315,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
if tmp_rm_data.get('rc', 0) != 0:
|
||||
display.warning('Error deleting remote temporary files (rc: %s, stderr: %s})'
|
||||
% (tmp_rm_res.get('rc'), tmp_rm_res.get('stderr', 'No error string available.')))
|
||||
else:
|
||||
self._connection._shell.tempdir = None
|
||||
|
||||
def _transfer_file(self, local_path, remote_path):
|
||||
self._connection.put_file(local_path, remote_path)
|
||||
@@ -485,13 +491,19 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
'''
|
||||
Get information from remote file.
|
||||
'''
|
||||
if tmp is not None:
|
||||
display.warning('_execute_remote_stat no longer honors the tmp parameter. Action'
|
||||
' plugins should set self._connection._shell.tempdir to share'
|
||||
' the tempdir')
|
||||
del tmp # No longer used
|
||||
|
||||
module_args = dict(
|
||||
path=path,
|
||||
follow=follow,
|
||||
get_checksum=checksum,
|
||||
checksum_algo='sha1',
|
||||
)
|
||||
mystat = self._execute_module(module_name='stat', module_args=module_args, task_vars=all_vars, tmp=tmp, delete_remote_tmp=(tmp is None),
|
||||
mystat = self._execute_module(module_name='stat', module_args=module_args, task_vars=all_vars,
|
||||
wrap_async=False)
|
||||
|
||||
if mystat.get('failed'):
|
||||
@@ -652,17 +664,24 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
update.update(options)
|
||||
self.connection.set_options(update)
|
||||
|
||||
def _execute_module(self, module_name=None, module_args=None, tmp=None, task_vars=None, persist_files=False, delete_remote_tmp=True, wrap_async=False):
|
||||
def _execute_module(self, module_name=None, module_args=None, tmp=None, task_vars=None, persist_files=False, delete_remote_tmp=None, wrap_async=False):
|
||||
'''
|
||||
Transfer and run a module along with its arguments.
|
||||
'''
|
||||
if tmp is not None:
|
||||
display.warning('_execute_module no longer honors the tmp parameter. Action plugins'
|
||||
' should set self._connection._shell.tempdir to share the tempdir')
|
||||
del tmp # No longer used
|
||||
if delete_remote_tmp is not None:
|
||||
display.warning('_execute_module no longer honors the delete_remote_tmp parameter.'
|
||||
' Action plugins should check self._connection._shell.tempdir to'
|
||||
' see if a tempdir existed before they were called to determine'
|
||||
' if they are responsible for removing it.')
|
||||
del delete_remote_tmp # No longer used
|
||||
|
||||
if task_vars is None:
|
||||
task_vars = dict()
|
||||
|
||||
remote_module_path = None
|
||||
args_file_path = None
|
||||
remote_files = []
|
||||
|
||||
# if a module name was not specified for this execution, use the action from the task
|
||||
if module_name is None:
|
||||
module_name = self._task.action
|
||||
@@ -677,21 +696,22 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
if not shebang and module_style != 'binary':
|
||||
raise AnsibleError("module (%s) is missing interpreter line" % module_name)
|
||||
|
||||
if not self._is_pipelining_enabled(module_style, wrap_async):
|
||||
tempdir = self._connection._shell.tempdir
|
||||
remote_module_path = None
|
||||
|
||||
# we might need remote tmp dir
|
||||
if not tmp:
|
||||
if not self._connection._shell.tempdir or tmp is None or 'tmp' not in tmp:
|
||||
tmp = self._make_tmp_path()
|
||||
else:
|
||||
tmp = self._connection._shell.tempdir
|
||||
if not self._is_pipelining_enabled(module_style, wrap_async):
|
||||
# we might need remote temp dir
|
||||
if tempdir is None:
|
||||
self._make_tmp_path()
|
||||
tempdir = self._connection._shell.tempdir
|
||||
|
||||
remote_module_filename = self._connection._shell.get_remote_filename(module_path)
|
||||
remote_module_path = self._connection._shell.join_path(tmp, remote_module_filename)
|
||||
remote_module_path = self._connection._shell.join_path(tempdir, remote_module_filename)
|
||||
|
||||
args_file_path = None
|
||||
if module_style in ('old', 'non_native_want_json', 'binary'):
|
||||
# we'll also need a temp file to hold our module arguments
|
||||
args_file_path = self._connection._shell.join_path(tmp, 'args')
|
||||
args_file_path = self._connection._shell.join_path(tempdir, 'args')
|
||||
|
||||
if remote_module_path or module_style != 'new':
|
||||
display.debug("transferring module to remote %s" % remote_module_path)
|
||||
@@ -712,8 +732,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
|
||||
environment_string = self._compute_environment_string()
|
||||
|
||||
if tmp and remote_module_path:
|
||||
remote_files = [tmp, remote_module_path]
|
||||
remote_files = []
|
||||
if tempdir and remote_module_path:
|
||||
remote_files = [tempdir, remote_module_path]
|
||||
|
||||
if args_file_path:
|
||||
remote_files.append(args_file_path)
|
||||
@@ -727,7 +748,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
(async_module_style, shebang, async_module_data, async_module_path) = self._configure_module(module_name='async_wrapper', module_args=dict(),
|
||||
task_vars=task_vars)
|
||||
async_module_remote_filename = self._connection._shell.get_remote_filename(async_module_path)
|
||||
remote_async_module_path = self._connection._shell.join_path(tmp, async_module_remote_filename)
|
||||
remote_async_module_path = self._connection._shell.join_path(tempdir, async_module_remote_filename)
|
||||
self._transfer_data(remote_async_module_path, async_module_data)
|
||||
remote_files.append(remote_async_module_path)
|
||||
|
||||
@@ -749,7 +770,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
# maintain a fixed number of positional parameters for async_wrapper
|
||||
async_cmd.append('_')
|
||||
|
||||
if not self._should_remove_tmp_path(tmp):
|
||||
if not self._should_remove_tmp_path(tempdir):
|
||||
async_cmd.append("-preserve_tmp")
|
||||
|
||||
cmd = " ".join(to_text(x) for x in async_cmd)
|
||||
@@ -763,7 +784,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
|
||||
|
||||
cmd = self._connection._shell.build_module_command(environment_string, shebang, cmd, arg_path=args_file_path).strip()
|
||||
|
||||
# Fix permissions of the tmp path and tmp files. This should be called after all files have been transferred.
|
||||
# Fix permissions of the tempdir path and tempdir files. This should be called after all
|
||||
# files have been transferred.
|
||||
if remote_files:
|
||||
# remove none/empty
|
||||
remote_files = [x for x in remote_files if x]
|
||||
|
||||
Reference in New Issue
Block a user