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

Support expect raw responses #80173

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from

Conversation

sbettid
Copy link
Contributor

@sbettid sbettid commented Mar 8, 2023

SUMMARY

Fixes #80075.

The idea, as mentioned in the comment in the related issue, is to add the possibility to specify also raw_responses, namely responses where the newline won't be added to the response and so suitable to all the cases in which the prompt expects a single character and immediately continues its execution after reading it, avoiding so the newline character to be read as answer of the next prompt.

In the module, I thought about extracting the processing of the responses to a function, which accepts as parameter if it needs to add the newline to the processed responses or not, since most of the processing is equal in both cases and only the (possibly) appended character changes.

I added the new parameter in the related documentation with a couple of examples, and, since this is my first feature contribution, I hope I did not miss any step on this.

Regarding the test, it was personally more challenging since I tried to work on a cross-platform solution which was not depending on external libraries, but I will be more than happy to improve it based on your feedback.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

expect module

ADDITIONAL INFORMATION

Compared output:

6c6
< changed: [localhost] => {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python3"}, "changed": true, "cmd": "bash my_command.sh", "delta": "0:00:00.205292", "end": "2023-03-08 20:40:34.974079", "rc": 0, "start": "2023-03-08 20:40:34.768787", "stdout": "enter first line: first line\r\nconfirmed: y", "stdout_lines": ["enter first line: first line", "confirmed: y"]}
---
> changed: [localhost] => {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python3"}, "changed": true, "cmd": "bash my_command.sh", "delta": "0:00:00.205315", "end": "2023-03-08 20:40:52.330082", "rc": 0, "start": "2023-03-08 20:40:52.124767", "stdout": "enter first line: first line\r\nconfirmed: y^J", "stdout_lines": ["enter first line: first line", "confirmed: y^J"]}

@ansibot ansibot added affects_2.15 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Mar 8, 2023
@ansibot
Copy link
Contributor

ansibot commented Mar 8, 2023

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

lib/ansible/modules/expect.py:0:0: option-incorrect-version-added: version_added for new option (raw_responses) should be '2.15'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 8, 2023
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 8, 2023
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Mar 9, 2023
@mattclay mattclay requested a review from nitzmahone March 9, 2023 19:20
@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 Mar 17, 2023
timeout = module.params['timeout']
echo = module.params['echo']

# one between responses and raw_responses needs to be specified
if responses is None and raw_responses is None:
module.fail_json(msg="responses (or raw_responses) is required")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this block, use required_one_of for AnsibleModule:

module = AnsibleModule(
    argument_spec=...,
    required_one_of=[['responses', 'raw_responses']],
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sivel for the feedback, I have updated the PR to implement your suggestion!

@ansibot ansibot removed 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 Mar 23, 2023
@sbettid sbettid requested review from sivel and removed request for nitzmahone March 24, 2023 07:06
@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 Apr 1, 2023
@ansibot ansibot removed 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 Apr 25, 2023
@sbettid
Copy link
Contributor Author

sbettid commented Apr 25, 2023

@sivel I have just updated the version added attribute based on the next release, please let me know if and how I can improve the PR. Thank you in advance!

@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 May 3, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 9, 2023
@juresaht2
Copy link

I would need this functionality. What can we do to help move it along?

@sbettid
Copy link
Contributor Author

sbettid commented Jan 16, 2024

Hi @juresaht2! Honestly I am a little bit unsure on how to proceed here. For sure I can try to rebase it in the next few days so that it will be aligned again with the target branch and back to "able to merge" status, although it will need to be reviewed first.

@juresaht2
Copy link

I've tried making an issue to Red Hat to get their attention, but apparently we'd need a subscription to one of their Ansible products for them to even look at this outside of techsupportistan, so we're stuck waiting for someone with maintainer privileges to merge the code or at least provide feedback.

I suppose technically one could modify the local expect.py file with your version.

@sbettid
Copy link
Contributor Author

sbettid commented Jan 17, 2024

I've tried making an issue to Red Hat to get their attention, but apparently we'd need a subscription to one of their Ansible products for them to even look at this outside of techsupportistan, so we're stuck waiting for someone with maintainer privileges to merge the code or at least provide feedback.

I suppose technically one could modify the local expect.py file with your version.

Yes I agree. In any case, to keep things aligned, I will rebase the branch with the target one by this weekend.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 18, 2024
@ansibot
Copy link
Contributor

ansibot commented Jan 18, 2024

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

lib/ansible/modules/expect.py:0:0: doc-required-mismatch: Argument 'responses' in argument_spec is not required, but is documented as being required

click here for bot help

@ansibot ansibot removed 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 Jan 18, 2024
@sbettid
Copy link
Contributor Author

sbettid commented Jan 18, 2024

@juresaht2 I have rebased the PR so that now it is still aligned with the target branch and resolving the conflicts that were present.

@ansibot ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Jan 18, 2024
@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 Feb 1, 2024
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.15 feature This issue/PR relates to a feature request. has_issue module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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.

expect module option - do not send enter/newline at the end of command option
5 participants