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.

Sign up for GitHub

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

Jump to bottom

Restore ansible --version output #55728

Merged
merged 8 commits into from
Apr 29, 2019
Merged

Restore ansible --version output #55728

merged 8 commits into from
Apr 29, 2019

Conversation

samdoran
Copy link
Contributor

SUMMARY

Fixes #55710

The default formatting class used by argparse strips newline characters, resulting in the output of ansible --version being a single string.

Implement a custom action to print out the already formatted version string and add tests to prevent future regressions.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

/lib/ansible/cli/arguments/option_helpers.py

ADDITIONAL INFORMATION

This only affects 2.9.

@samdoran samdoran requested a review from sivel April 24, 2019 19:01
@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 24, 2019
AlanCoding
AlanCoding reviewed Apr 24, 2019
View reviewed changes
test/integration/targets/ansible/test_version.py Outdated
version_lines = version_string.splitlines()

assert len(version_lines) == 6, 'Incorrect number of lines in "ansible --version" output'
assert re.match(b'ansible [0-9.a-z]+$', version_lines[0]), 'Incorrect ansible version line in "ansible --version" output'
Copy link
Member

Choose a reason for hiding this comment

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

👍 this should cover it, thanks!

@samdoran samdoran changed the title Restore ansible --version output [WIP] Restore ansible --version output Apr 24, 2019
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Apr 24, 2019
@abadger
Copy link
Contributor

abadger commented Apr 24, 2019

I think this is what you need: https://docs.python.org/2/library/argparse.html#prog

prog should be set to: os.path.basename(self.args[0]). you'll need to pass that from the Cli object to create_base_parser() as a new function parameter.

@AlanCoding
Copy link
Member

I pulled this branch and got:

(ansible3) arominge-OSX:ansible alancoding$ ansible --version
ansible 2.9.0.dev0
  config file = None
  configured module search path = ['/Users/alancoding/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/alancoding/Documents/repos/ansible/lib/ansible
  executable location = /Users/alancoding/.virtualenvs/ansible3/bin/ansible
  python version = 3.6.5 (default, Apr 25 2018, 14:23:58) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.1)]

Looks good!

@samdoran
Improvements to unit test
0f5bf91 
- Fix capsys error on Python 2.6
- Patch sys.argv in order to get the correct value in version output
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Apr 25, 2019
samdoran
samdoran commented Apr 26, 2019
View reviewed changes
lib/ansible/cli/__init__.py Outdated Show resolved Hide resolved
@samdoran
Require list of args for CLI object, require prog
d6b433e 
Add sanity test on CLI constructor to ensure args is defined.
Make prog a required parameter of create_base_parser() and update all uses to pass in the newly required paramater.
Update unit tests to reflect and test these changes.
@samdoran
Fix a few more places where an empty list is no longer acceptable
4fb1087
samdoran
samdoran commented Apr 29, 2019
View reviewed changes
docs/bin/generate_man.py Outdated Show resolved Hide resolved
@ansibot
Copy link
Contributor

ansibot commented Apr 29, 2019

cc @acozine
click here for bot help

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Apr 29, 2019
@samdoran samdoran changed the title [WIP] Restore ansible --version output Restore ansible --version output Apr 29, 2019
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Apr 29, 2019
@samdoran samdoran merged commit b3ce3fc into ansible:devel Apr 29, 2019
@samdoran samdoran deleted the issue/55710-version-newline branch April 29, 2019 20:38
ndclt pushed a commit to ndclt/ansible that referenced this pull request Jun 13, 2019
@samdoran @ndclt
Restore ansible --version output (ansible#55728)
01af89f 
* Add custom action class for version info
* Use args from CLI as prog for ArgumentParser object
* Make prog a required parameter of create_base_parser() and update all uses to pass in the newly required parameter.
* Add unit test for checking ansible --version
* Update other related unit tests
@ansible ansible locked and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel left review comments

@abadger abadger abadger left review comments

@AlanCoding AlanCoding AlanCoding approved these changes

Assignees
No one assigned
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ansible --version no longer prints version on new line
6 participants
@samdoran @abadger @AlanCoding @ansibot @sivel @bcoca