File refactor #39345

abadger · 2018-04-26T04:26:08Z

SUMMARY

Refactoring file so that it is easier to:

fix bugs. Right now, fixing a bug for one use case has unintended consequences for other uses
unittest permutations. There are many cornercases with file. Testing it with integration tests becomes really time consuming. It would be much better to be able to test this using pytest. But to do that we need to splt functionality apart into testable chunks.

A few changes may also spill over to other file handling modules like copy which depend on the file module to perform their work sometimes.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

primarily, lib/ansible/modules/files/file.py

ANSIBLE VERSION

devel

ADDITIONAL INFORMATION

This has been okayed for inclusion in Ansible-2.6 as it is required to sanely bugfix the file module.

ansibot · 2018-04-26T05:29:06Z

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/module_utils/basic.py:232:45: E261 at least two spaces before inline comment

click here for bot help

Some file_common_args are not common to all file modules. Divide the args into categories for a future refator of this into a separate file_argspec.

* Remove use of six.b as Python-2.6+ have byte literals. * Make AnsibleModule a global object so we'll have access to it in all the functions we're going to break this up into. * Rework the parameters so things that are in file_common_args are used from file_common_args or the reason for deviation is documented. * Remove validate as a parameter: this should be taken care of by removing it from params before the copy and template action plugin invoke file. * Rename diff_peek to _diff_peek as it is an internal parameter. * add module_name execute_module call to assemble so that it is more greppable

abadger · 2018-05-08T00:59:49Z

This is ready for review from @mattclay (as the 2.6 RM). This mostly moves the logic around without changing too much. A few refactors get their start here but will be completed in future PRs.

mattclay · 2018-05-08T19:46:23Z

lib/ansible/modules/files/file.py

+        raise ParameterError(results={"path": params["path"],
+                             "msg": "recurse option requires state to be 'directory'"})
+
+    return params


Since this function mutates the input params why return it? Did you intend to copy it before modifying it?

Conceptually this is additions to the argspec so it should be mutating the params and not returning the params. I added the return since it didn't hurt anything and the calling code immediately does the equivalent of params = module.params I can remove the return and change the caller if you want.

Yes, please remove the return and update the caller.

mattclay

Aside from the aforementioned change request, everything else looks OK.

* Separate the logic for each state into separate functions * Start the process of separating out initialization (pre-processing of parameters that cannot be done via arg spec) from the logic to implement each state. * Start the process of raising exceptions for errors and returning result values from each state implementing function Goal is for all fail_json's to be consolidated into exception handlers at the toplevel and for there to be only one exit_json() at the toplevel.

gundalow · 2018-05-10T10:19:59Z

lib/ansible/modules/files/file.py

+        argument_spec=dict(
+            state=dict(choices=['file', 'directory', 'link', 'hard', 'touch', 'absent'], default=None),
+            path=dict(aliases=['dest', 'name'], required=True, type='path'),
+            original_basename=dict(required=False),  # Internal use only, for recursive ops


Should this be _original_basename to match _diff_peek?

original_basename is used for more than one module in several action_plugins. I'll have to look at each place to see if they should all be made private or if certain ones call file while others are calling (for instance) copy.

gundalow · 2018-05-10T10:23:22Z

lib/ansible/module_utils/basic.py

@@ -216,25 +216,32 @@
 _ANSIBLE_ARGS = None

 FILE_COMMON_ARGUMENTS = dict(
-    src=dict(),
+    # These are things we want. About setting metadata (mode, ownership, permissions in general) on


Wonder if it's worth giving some details of how to know if something should be in here or not, ie what is "common"?
May help reduce confusion in the future and things like backup, force being added back in

Yeah. I can do that when we actually break basic.py apart.

gundalow · 2018-05-10T10:44:46Z

lib/ansible/module_utils/basic.py

+
+    # not taken by the file module, but other action plugins call the file module so this ignores
+    # them for now. In the future, the caller should take care of removing these from the module
+    # arugments before calling the file module.


arugments -> arguments

k. I'll fix that in a followup PR

abadger force-pushed the file-refactor branch from 0387896 to e92c29e Compare April 26, 2018 05:47

jborean93 removed the needs_triage Needs a first human triage before being processed. label Apr 26, 2018

abadger force-pushed the file-refactor branch 2 times, most recently from 91ed5fd to e65beb1 Compare May 4, 2018 04:32

abadger added 3 commits May 4, 2018 14:26

Categorize FILE_COMMON_ARGS

86347e1

Some file_common_args are not common to all file modules. Divide the args into categories for a future refator of this into a separate file_argspec.

Make the copy module pass correct parameters to file module

2189db5

abadger force-pushed the file-refactor branch 2 times, most recently from cc17bac to 1d8cb9d Compare May 7, 2018 22:27

abadger changed the title ~~[WIP] File refactor~~ File refactor May 8, 2018

abadger removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label May 8, 2018

abadger mentioned this pull request May 8, 2018

File refactor2 #39834

Merged

ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label May 8, 2018

mattclay reviewed May 8, 2018

View reviewed changes

mattclay approved these changes May 8, 2018

View reviewed changes

abadger force-pushed the file-refactor branch from 1d8cb9d to 10f0bcc Compare May 8, 2018 21:20

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 8, 2018

gundalow reviewed May 10, 2018

View reviewed changes

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 10, 2018

abadger merged commit 4e2876b into ansible:devel May 10, 2018

abadger deleted the file-refactor branch May 10, 2018 21:48

sivel mentioned this pull request Jul 8, 2018

[2.6][file module] lost validate option? #42150

Closed

ansible locked and limited conversation to collaborators May 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File refactor #39345

File refactor #39345

abadger commented Apr 26, 2018

ansibot commented Apr 26, 2018

abadger commented May 8, 2018

mattclay May 8, 2018

abadger May 8, 2018

mattclay May 8, 2018

mattclay left a comment

gundalow May 10, 2018

abadger May 11, 2018

gundalow May 10, 2018

abadger May 10, 2018

gundalow May 10, 2018

abadger May 10, 2018

File refactor #39345

File refactor #39345

Conversation

abadger commented Apr 26, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Apr 26, 2018

abadger commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment