Skip to content
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

Fix disabled timeouts in ansible.builtin.expect #80983

Closed
wants to merge 1 commit into from

Conversation

brianhelba
Copy link

SUMMARY

Fixes #80982. See that issue for a more complete description of the problem.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible.builtin.expect

Fixes ansible#80982. See that issue for a more complete description of the problem.
@ansibot ansibot added affects_2.16 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch labels Jun 6, 2023
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jun 7, 2023
@ansibot
Copy link
Contributor

ansibot commented Jun 7, 2023

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/expect.py:168:7: E114: indentation is not a multiple of 4 (comment)
lib/ansible/modules/expect.py:169:7: E111: indentation is not a multiple of 4

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jun 7, 2023
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jun 8, 2023
@jborean93
Copy link
Contributor

This may be a breaking change for people who already do timeout: 0. It might be better to investigate how it used to work in the past and what broke it. It could potentially be the change in 694c11d but that would have to be verified.

@s-hertel
Copy link
Contributor

s-hertel commented Jun 8, 2023

Since pexpect allows a timeout of 0 (no wait) in addition to None (wait indefinitely), a backwards compatible option might be to make timeout type raw instead and validate that it's None or an int in the module:

diff
diff --git a/lib/ansible/modules/expect.py b/lib/ansible/modules/expect.py
index c9aac71ac5..120192f71e 100644
--- a/lib/ansible/modules/expect.py
+++ b/lib/ansible/modules/expect.py
@@ -43,7 +43,7 @@ options:
       responses. List functionality is new in 2.1.
   required: true
 timeout:
-    type: int
+    type: raw
   description:
     - Amount of time in seconds to wait for the expected strings. Use
       C(null) to disable timeout.
@@ -122,6 +122,7 @@ except ImportError:

from ansible.module_utils.basic import AnsibleModule, missing_required_lib
from ansible.module_utils.common.text.converters import to_bytes, to_native
+from ansible.module_utils.common.validation import check_type_int


def response_closure(module, question, responses):
@@ -147,7 +148,7 @@ def main():
           creates=dict(type='path'),
           removes=dict(type='path'),
           responses=dict(type='dict', required=True),
-            timeout=dict(type='int', default=30),
+            timeout=dict(type='raw', default=30),
           echo=dict(type='bool', default=False),
       )
   )
@@ -162,6 +163,11 @@ def main():
   removes = module.params['removes']
   responses = module.params['responses']
   timeout = module.params['timeout']
+  if timeout is not None:
+      try:
+          timeout = check_type_int(timeout)
+      except TypeError as te:
+          module.fail_json(msg=to_native(te))
   echo = module.params['echo']

   events = dict()

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 16, 2023
@ansibot ansibot added has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2023
@s-hertel
Copy link
Contributor

Thanks for the bug report. This has been fixed in #82522.

@s-hertel s-hertel closed this Jan 26, 2024
@ansible ansible locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.16 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. has_issue module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ansible.builtin.expect does not actually support "null" timeout
5 participants