Fix ctrl+c in pause module and make other improvements #40134

samdoran · 2018-05-15T05:44:20Z

SUMMARY

Ctrl + C works in all scenarios.

Use curses to control stdout to avoid displaying control characters on screen and make interactive input more user friendly.

Validate the echo parameter and ensure it is always cast as a bool.

Add integration tests using pexpect.

Supercedes #38049

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

pause.py

ANSIBLE VERSION

2.6

now ctrl+c works in most scenarios: - naked: - pause: - with prompt - pause: prompt=hi - time wait - pause: seconds=60 - time wait with prompt - pause: seconds=60 prompt=hi removed redundant seconds if fixes ansible#35372

ansibot · 2018-05-15T22:24:46Z

cc @mattclay @tbielawa
click here for bot help

ansibot · 2018-05-15T23:01:48Z

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

test/integration/targets/pause/runme.py:27:1: E265 block comment should start with '# '
test/integration/targets/pause/runme.py:83:1: E266 too many leading '#' for block comment
test/integration/targets/pause/runme.py:83:1: E303 too many blank lines (3)
test/integration/targets/pause/runme.py:139:1: E266 too many leading '#' for block comment
test/integration/targets/pause/runme.py:139:1: E303 too many blank lines (3)
test/integration/targets/pause/runme.py:195:1: E266 too many leading '#' for block comment
test/integration/targets/pause/runme.py:195:1: E303 too many blank lines (3)
test/integration/targets/pause/runme.py:254:1: E266 too many leading '#' for block comment
test/integration/targets/pause/runme.py:254:1: E303 too many blank lines (3)
test/integration/targets/pause/runme.py:270:26: E226 missing whitespace around arithmetic operator

The test ansible-test sanity --test use-compat-six [explain] failed with 1 error:

test/integration/targets/pause/runme.py:8:1: use `ansible.module_utils.six` instead of `six`

click here for bot help

mattclay · 2018-05-16T07:47:31Z

test/integration/targets/pause/runme.sh

+
+export ANSIBLE_ROLES_PATH=./roles
+
+python runme.py -i ../../inventory "$@"


You shouldn't need a runme.sh just to run runme.py, since ansible-test will execute runme.py directly.

However, to avoid having to install pexpect for every integration test run via integration.txt, it may be better to keep this for now and install pexepect via pip here rather than using integration.txt.

Thanks for answering that! I was going to ask you about that today but you beat me to it.

PEP8 fixes in test-pause.py

samdoran · 2018-05-16T12:32:03Z

@mattclay One thing I've noticed is that inside the container I don't have a tty to query for the appropriate VERASE. Would there be a way to add a tty to the container by running it with -it or -t?

Traceback (most recent call last):
  File "test-pause.py", line 17, in <module>
    backspace = termios.tcgetattr(sys.stdin.fileno())[6][termios.VERASE]
termios.error: (25, 'Inappropriate ioctl for device')

ansibot · 2018-05-16T13:09:16Z

The test ansible-test sanity --test use-compat-six [explain] failed with 1 error:

test/integration/targets/pause/test-pause.py:8:1: use `ansible.module_utils.six` instead of `six`

click here for bot help

Use six from ansible.module_utils.six Use try except when trying to determien erase sequence to account for lack of TTY in containers

mattclay · 2018-05-16T21:37:42Z

@samdoran I'll try adding -it when running all test containers to see if it causes any issues.

samdoran · 2018-05-17T14:30:34Z

@mattclay I was able to work around that with a try except statement that hardcodes a backspace escape sequence and that seems to be working.

The last test problem I need to solve is that I am testing aborting the play interactively, which causes the tests to report failure (well, unstable actually). I thought I put the correct string in the task name, but that doesn't seem to fix it. This may be different than other tests in that I am actually aborting the play, but I'm not sure.

mattclay · 2018-05-17T14:52:16Z

@samdoran Your latest changes to use EXPECTED FAILURE should fix the unstable test results.

mattclay · 2018-05-17T15:48:30Z

It's good that you have a work around, because both -it and -t cause problems for CI.

samdoran · 2018-05-17T18:10:45Z

Thanks for looking into it.

Accept two digit precision for pause length in test Check for seconds when seconds is specificed, minutes when minutes is specified

* Fix all cases with pause and ctrl+c - naked: - pause: - with prompt - pause: prompt=hi - time wait - pause: seconds=60 - time wait with prompt - pause: seconds=60 prompt=hi Fixes ansible#35372 * Use curses to control stdout * Use curses to clear lines on interactive input * Validate input for echo parameter and fail nicely if invalid * Add integration tests for pause module using pexpect * Use try except when trying to determine erase sequence to account for lack of TTY in containers in tests * Improve output validation for regular paus test * Accept two digit precision for pause length in test * Check for seconds when seconds is specificed, minutes when minutes is specified * Add test for no TTY mode Co-authored by: Toshio Kuratomi <a.badger@gmail.com> Co-authored by: Brian Coca <brian.coca+git@gmail.com> (cherry picked from commit 1c20029)

Backport # 40134 - Ensure Ctrl+C interrupt for pause module works in all cases

* Fix all cases with pause and ctrl+c - naked: - pause: - with prompt - pause: prompt=hi - time wait - pause: seconds=60 - time wait with prompt - pause: seconds=60 prompt=hi Fixes ansible#35372 * Use curses to control stdout * Use curses to clear lines on interactive input * Validate input for echo parameter and fail nicely if invalid * Add integration tests for pause module using pexpect * Use try except when trying to determine erase sequence to account for lack of TTY in containers in tests * Improve output validation for regular paus test * Accept two digit precision for pause length in test * Check for seconds when seconds is specificed, minutes when minutes is specified * Add test for no TTY mode Co-authored by: Toshio Kuratomi <a.badger@gmail.com> Co-authored by: Brian Coca <brian.coca+git@gmail.com>

bcoca and others added 5 commits May 15, 2018 01:34

fix all cases with pause and ctrl+c

1ec2f20

now ctrl+c works in most scenarios: - naked: - pause: - with prompt - pause: prompt=hi - time wait - pause: seconds=60 - time wait with prompt - pause: seconds=60 prompt=hi removed redundant seconds if fixes ansible#35372

Go back to original approach with minor improvements

b749d50

Use curses to control stdout

86d8a02

Use curses to clear lines on interactive input

e10d73c

Validate input for echo parameter and fail nicely if invalid

216d0ba

samdoran mentioned this pull request May 15, 2018

fix all cases with pause and ctrl+c #38049

Closed

samdoran removed the needs_triage Needs a first human triage before being processed. label May 15, 2018

Add integration tests for pause module using pexpect

3127363

samdoran changed the title ~~[WIP] Fix ctrl+c in pause module and make other improvements~~ Fix ctrl+c in pause module and make other improvements May 15, 2018

ansibot added test This PR relates to tests. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels May 15, 2018

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 15, 2018

mattclay reviewed May 16, 2018

View reviewed changes

samdoran added 2 commits May 16, 2018 08:28

Remove pexpect from integration.txt

c871a80

Install pexpect via runme.sh

e69a351

PEP8 fixes in test-pause.py

samdoran added 2 commits May 16, 2018 11:34

Fix test errors

22def34

Use six from ansible.module_utils.six Use try except when trying to determien erase sequence to account for lack of TTY in containers

Expect failure in pause playbooks

0b41c2c

samdoran added this to To Do in 2.5.x blocker list via automation May 17, 2018

Try expected failure for task name that should fail

643dbad

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 17, 2018

samdoran added 2 commits May 17, 2018 12:22

Also test a standard playbook

10d8d1e

Add changelog entry

b433866

samdoran removed this from To Do in 2.5.x blocker list May 17, 2018

samdoran added 2 commits May 18, 2018 13:41

Improve output validation for regular paus test

ebdf9d9

Accept two digit precision for pause length in test Check for seconds when seconds is specificed, minutes when minutes is specified

Add test for no TTY mode

e0c8c0d

ansibot added the affects_2.6 This issue/PR affects Ansible v2.6 label May 18, 2018

samdoran merged commit 1c20029 into ansible:devel May 21, 2018

samdoran deleted the issue/35372-pause-ctrl-c branch May 21, 2018 14:04

samdoran mentioned this pull request May 22, 2018

Backport # 40134 - Ensure Ctrl+C interrupt for pause module works in all cases #40480

Merged

samdoran added a commit that referenced this pull request May 22, 2018

Fix ctrl+c in pause module and add tests (#40134) (#40480)

dbcecb6

Backport # 40134 - Ensure Ctrl+C interrupt for pause module works in all cases

zanglang mentioned this pull request Jun 27, 2018

"pause" module fails with NameError when "curses" is not installed #42004

Closed

sivel mentioned this pull request Aug 8, 2018

Allow user to break out of pause-prompt #43822

Closed

ansible locked and limited conversation to collaborators May 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ctrl+c in pause module and make other improvements #40134

Fix ctrl+c in pause module and make other improvements #40134

samdoran commented May 15, 2018 •

edited

ansibot commented May 15, 2018

ansibot commented May 15, 2018

mattclay May 16, 2018

mattclay May 16, 2018

samdoran May 16, 2018

samdoran commented May 16, 2018

ansibot commented May 16, 2018

mattclay commented May 16, 2018

samdoran commented May 17, 2018

mattclay commented May 17, 2018

mattclay commented May 17, 2018

samdoran commented May 17, 2018 •

edited


		export ANSIBLE_ROLES_PATH=./roles

		python runme.py -i ../../inventory "$@"

Fix ctrl+c in pause module and make other improvements #40134

Fix ctrl+c in pause module and make other improvements #40134

Conversation

samdoran commented May 15, 2018 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented May 15, 2018

ansibot commented May 15, 2018

mattclay May 16, 2018

Choose a reason for hiding this comment

mattclay May 16, 2018

Choose a reason for hiding this comment

samdoran May 16, 2018

Choose a reason for hiding this comment

samdoran commented May 16, 2018

ansibot commented May 16, 2018

mattclay commented May 16, 2018

samdoran commented May 17, 2018

mattclay commented May 17, 2018

mattclay commented May 17, 2018

samdoran commented May 17, 2018 • edited

samdoran commented May 15, 2018 •

edited

samdoran commented May 17, 2018 •

edited