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

unarchive: fix non-english locales #76542

Merged
merged 6 commits into from Dec 14, 2021
Merged

Conversation

joneuhauser
Copy link
Contributor

@joneuhauser joneuhauser commented Dec 11, 2021

SUMMARY

For GNU Gettext, the LANGUAGE environment variable takes precedence over LANG or LC_ALL (http://www.gnu.org/savannah-checkouts/gnu/gettext/manual/html_node/The-LANGUAGE-variable.html#The-LANGUAGE-variable). On systems where LANGUAGE was set to a non-english locale, the output of the tar command was in that language, so the module didn't understand it. This lead to the module failing silently ("changed": false, but the archive was not extracted).

The issue is simply fixed by adding the LANGUAGE environment variable as well.

Tested with:

ansible [core 2.12.1]
  config file = /md1/config/ansible/ansible.cfg
  configured module search path = ['/user/hi202/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/ansible/lib/python3.8/site-packages/ansible
  ansible collection location = /user/hi202/.ansible/collections:/usr/share/ansible/collections
  executable location = /opt/ansible/bin/ansible
  python version = 3.8.0 (default, Feb 25 2021, 22:10:10) [GCC 8.4.0]
  jinja version = 3.0.3
  libyaml = True

on Ubuntu 20.04, German system, i.e.

LANG=de_DE.UTF-8
LANGUAGE=de_DE:en
LC_MESSAGES="de_DE.UTF-8"

If you are affected by this issue, add

environment:
        LANGUAGE: en_US.utf8

to your play.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

unarchive

ADDITIONAL INFORMATION

Step by step reproduction:

  1. Change locale as above
  2. Try to unarchive a tar archive using one of the example plays in the documentation
  3. Nothing is unarchived, the module shows "changed: false"

For GNU Gettext, the LANGUAGE environment variable takes precedence over LANG or LC_ALL. On systems where LANGUAGE was set to a non-english locale, the output of the tar command therefore not understood and the module failed silently ("changed": false, but the archive was not extracted).
@ansibot ansibot added affects_2.13 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. 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. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Dec 11, 2021
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

Could you add a changelog and test for this? For the test you can copy https://github.com/ansible/ansible/blob/devel/test/integration/targets/unarchive/tasks/test_non_ascii_filename.yml#L32-L66 and set the LANGUAGE env var instead.

@joneuhauser
Copy link
Contributor Author

Thanks for having a look!

For the test you can copy https://github.com/ansible/ansible/blob/devel/test/integration/targets/unarchive/tasks/test_non_ascii_filename.yml#L32-L66 and set the LANGUAGE env var instead.

Alright, done. However it's a bit meaningless to set the LANGUAGE env var if the referenced language pack doesn't exist (and LANGUAGE=C yields the same result as LANGUAGE=en_US on my system). I added some code to ensure that the locale actually exists.

Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

Thanks for adding the changelog!

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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 Dec 14, 2021
@joneuhauser joneuhauser reopened this Dec 14, 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_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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 Dec 14, 2021
…ge_var.yml

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 14, 2021
@ansibot ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label Dec 14, 2021
@joneuhauser
Copy link
Contributor Author

@s-hertel Thanks for your review and assistance with my first MR here. I'll perform the changes you suggested also on my other MR that addresses the same issue.

@s-hertel s-hertel merged commit 49e1cb9 into ansible:devel Dec 14, 2021
@s-hertel
Copy link
Contributor

@joneuhauser Awesome first contribution, thanks for working on this!

@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Dec 14, 2021
bcoca pushed a commit to bcoca/ansible that referenced this pull request Dec 15, 2021
* unarchive: fix non-english locales

For GNU Gettext, the LANGUAGE environment variable takes precedence over LANG or LC_ALL. On systems where LANGUAGE was set to a non-english locale, the output of the tar command therefore not understood and the module failed silently ("changed": false, but the archive was not extracted).

* add tests

* changelog
@ansible ansible locked and limited conversation to collaborators Dec 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.13 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. 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.

None yet

3 participants