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

Add support for unicode in ansible-inventory CLI #74912

Merged
merged 5 commits into from Jun 10, 2021

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Jun 4, 2021

SUMMARY

Fixes #57378

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/cli/inventory.py

@ansibot ansibot added affects_2.12 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:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 4, 2021
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

code LGTM, would be nice to have test to avoid regressions

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 9, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 9, 2021
@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Jun 9, 2021
Copy link
Contributor

@abadger abadger left a comment

Choose a reason for hiding this comment

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

Ugh. I've looked at this file wholistically and it's unicode handling looks like it is very, very wrong. I think that it needs a lot of changes to work right. ping me on irc and I can walk through it with you.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and 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. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 10, 2021
@mkrizek
Copy link
Contributor Author

mkrizek commented Jun 10, 2021

So as a summary of what this PR does:

  • fixes ability to print unicode chracters for ansible-inventory --list and ansible-inventory --list --yaml commands and also --output (output to a file) for both formats on Python 2 and 3,
  • adds integration tests for all of above,
  • does NOT touch any --toml code - unicode works for --toml in devel branch on Python 3 correctly. On Python 2 --toml prints unicode characters incorrectly (in a form of \u1234) but that is an existing issue in devel branch so this PR does NOT break that - the reason I found out about that is because I added a test for --toml for completeness. In the end, the test is restricted for Python 3 only.

As per above, this PR is an improvement to the unicode support in ansible-inventory --list over what is currently in devel without breaking or removing any functionality and as such could be backported.

Certainly there is more to improve but I don't think we should hold off on merging this (unless someone thinks the PR as is is incorrect of course). We can follow up in another PR. We can also leave #57378 open.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jun 10, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 10, 2021
Copy link
Contributor

@abadger abadger left a comment

Choose a reason for hiding this comment

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

I discussed this with mkrizek on IRC and he fixed the major problem that I saw (writing the file as text which can traceback and could write the file in non-utf8 which ansible will not read correctly) and we also agreed on the other things that should be fixed in a followup(s) (fix use of to_native when calling display.*(), moving the toml routines to a [possibly private] utility file, moving the transformation of yaml output from bytes to text into AnsibleDumper).

We also discussed the toml output a bit. On Python2, the toml library both removes and adds backslashes that it shouldn't, which badly mangles the escaped strings. it would be possible to fix the problem in our toml_dumps function https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/toml.py#L118 by overriding the str, unicode (and AnsibleUnicode, AnsibleUnsafeBytes, and AnsibleUnsafeText) handlers in dump_funcs. We could also deprecate use of the toml library and promote use of tomlkit because tomlkit does the right thing here. However, the problem only occurs on Python2. The python3 repr functionality which the toml library relies on handles output of non-ascii characters in a non-supportive locale in a different way such that the toml library does not mangle those.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 10, 2021
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jun 10, 2021
@jborean93 jborean93 merged commit 5ac1b04 into ansible:devel Jun 10, 2021
@mkrizek mkrizek deleted the inventory-cli-unicode branch June 11, 2021 06:24
mkrizek added a commit to mkrizek/ansible that referenced this pull request Jun 15, 2021
@ansible ansible locked and limited conversation to collaborators Jul 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.12 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ansible-inventory doesn’t use unicode
7 participants