-
Notifications
You must be signed in to change notification settings - Fork 23.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds limit
parameter to ansible.builtin.find
#83153
base: devel
Are you sure you want to change the base?
Conversation
I appreciate you likely have more important work to deal with but tagging module author @bcoca, as it feels like ansibot should have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure this feature is needed as it is a simple thing to cut down the return of files to only those you need returned_files[:5]
.
Though this can be a performance improvement, it would only be really noticeable with huge amounts of files.
lib/ansible/modules/find.py
Outdated
- Set to V(-1) for unlimited matches. | ||
- Default is unlimited matches. | ||
type: int | ||
default: -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use None
for 'unset/unlimited'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change. Am I correct in thinking that the type should still be int
, given there's no Optional[int]
type (https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have changed. I'm not sure what the correct value to set as default in the docs should be now that it's None
(see test failure). Any help would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just remove it, None
is the default for default
. All types allow None
as a default as it is 'unset'.
lib/ansible/modules/find.py
Outdated
@@ -154,6 +154,15 @@ | |||
- When doing a C(contains) search, determine the encoding of the files to be searched. | |||
type: str | |||
version_added: "2.17" | |||
max_matches: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would rename limit
or 'maximum` as this does not relate to a 'match' field, but to all the conditions the other options can set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-named to limit
.
lib/ansible/modules/find.py
Outdated
@@ -154,6 +154,15 @@ | |||
- When doing a C(contains) search, determine the encoding of the files to be searched. | |||
type: str | |||
version_added: "2.17" | |||
max_matches: | |||
description: | |||
- Set the maximum number of matches to make before returning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more like:
The maximum number of matching paths returned, after finding this many the find action will stop looking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@@ -530,7 +553,8 @@ def handle_walk_errors(e): | |||
if not os.path.isdir(npath): | |||
raise Exception("'%s' is not a directory" % to_native(npath)) | |||
|
|||
for root, dirs, files in os.walk(npath, onerror=handle_walk_errors, followlinks=params['follow']): | |||
# Setting `topdown=True` to explicitly guarantee matches are made from the shallowest directory first | |||
for root, dirs, files in os.walk(npath, onerror=handle_walk_errors, followlinks=params['follow'], topdown=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this changes the normal search by adding topdown, this is not really backwards compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would even add this as an option before i limit matching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topdown
is True
by default (https://docs.python.org/3/library/os.html#os.walk) so it should not change the current behaviour. The intention was to make it explicit that the module is guaranteed to be topdown - perhaps the docs is a better place for this though?
I'm not sure how many people will have a use-case where they'd want to switch between topdown/bottomup. However, I'm happy to add it if you think it could be useful.
max_matches
parameter to ansible.builtin.find
limit
parameter to ansible.builtin.find
The test
|
@bcoca many thanks for reviewing. I have addressed most of your comments. Just two things left to resolve:
This is the exact situation that I have - lot's of files, some of them very large. The module isn't really viable for my use-case without this option. |
SUMMARY
ansible.builtin.find
currently looks for matches in all non-filtered files. When there is a very big file system in the search scope, and/or an expensive search is made (e.g. based off whole file contents), the operation can be very expensive.This PR adds the ability for the user to limit the number of matches that are made. Use cases that would benefit/be enabled with this functionality include:
> n
matching files in a directory.ISSUE TYPE
ADDITIONAL INFORMATION
Try the module using: