-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
locale_gen: fix: use glibc_ubuntu mode only if files for that exists #9735
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
CC @russoz
changelogs/fragments/9735-locale_gen-use_glibc_ubuntu_mode_only_if_file_exists.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @k0ste thanks for reporting the issue and submitting the PR.
I am the one who introduced those changes and, at the time, it was understood that the module supported only Ubuntu and/or Debian systems, so I am caught a bit surprised and unaware that it has been used in other systems as well. Keep in mind that the docs and the tests for the module support only Debian/Ubuntu and have been like that for a while long time - long before I revamped the module.
On a side note: I noticed you mentioned you have been using this for 8 years, but your system is CentOS Stream 9, so I assume that worked in Stream 8 and maybe even CentOS 7 before that?
That being said, of course it does make sense to keep supporting the module in OS'es where it worked before. The first thing I would ask from you in this PR is no remove or adjust the line https://github.com/ansible-collections/community.general/blob/main/tests/integration/targets/locale_gen/tasks/main.yml#L13 to also include CentOS Stream 9 and any other systems you see fit.
Right now the tests are passing for your PR because they detect the system is neither Ubuntu nor Debian and skip the actual testing logic.
I am particularly concerned about the PR changing the semantics for the RV glibc
, which is likely a breaking change. But we can address that after you deal with the integration tests.
changelogs/fragments/9735-locale_gen-use_glibc_ubuntu_mode_only_if_file_exists.yml
Outdated
Show resolved
Hide resolved
The original #!/usr/bin/python
# -*- coding: utf-8 -*-
import os
import os.path
from subprocess import Popen, PIPE, call
DOCUMENTATION = '''
---
module: locale_gen
short_description: Creates of removes locales.
description:
- Manages locales by editing /etc/locale.gen and invoking locale-gen.
version_added: "1.6"
options:
name:
description:
- Name and encoding of the locale, such as "en_GB.UTF-8".
required: true
default: null
aliases: []
state:
description:
- Whether the locale shall be present.
required: false
choices: ["present", "absent"]
default: "present"
'''
EXAMPLES = '''
# Ensure a locale exists.
- locale_gen: name=de_CH.UTF-8 state=present
''' The original version had no limitations, so we used the module in the c7, c8s, c9s and Archlinux Adjusted |
Thanks for the git spelunking :-) I certainly did not check the original versions of the module, but I am pretty sure the latest docs did not mention any of that, and the tests were not being executed for any of those distros. Thanks for adjusting the |
9784dfc
to
e6214d5
Compare
You are welcome. Tests added |
Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>
…y_if_file_exists.yml Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
54019cf
to
1f5695e
Compare
@k0ste This PR contains |
Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>
Removed preparation tasks for Archlinux. Now somebody need to take a look what with container glibc? |
The Arch Linux container had very limited locales (probably to save space), I've fixed that in ansible-community/images#110 and re-triggered the tests; they now pass. |
I'm getting the feeling that this PR is not needed at all (except the test part). RHEL, Fedora, and CentOS (I tried 7 and 8) do not have /etc/locale.gen, do not have the locale-gen CLI tool, so the module never worked on these platforms and this PR isn't changing that. The reason that this task: https://github.com/k0ste/ansible-role-linux_helper/tree/0f6241d4177a004ed2bbc989d65b89726ab7e0fd/tasks#L10-L16 seemed to have worked before is that the module never did anything, since /etc/locale.gen was templated to the expected value before running the module, which basically made the module a no-op. Simply removing the community.general.locale_gen module from that task is the right solution to fix the task. That task does not need the module (and never needed it). |
This statement is also not correct. |
The module does, like said before, |
The mechanism it uses to determine availability is to check in the file, in this case |
It would probably be helpful to have a separate module |
SUMMARY
Fixes #9734
ISSUE TYPE
COMPONENT NAME
locale_gen
ADDITIONAL INFORMATION
This PR fixes issue for another glibc distro. The implementation should remove ubuntu_mode at all, and use
localectl list-locales
for any systemd distro in present days IMO