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

locale_gen: fix: use glibc_ubuntu mode only if files for that exists #9735

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

k0ste
Copy link

@k0ste k0ste commented Feb 12, 2025

SUMMARY

Fixes #9734

ISSUE TYPE
  • Bugfix Pull Request
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

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Feb 12, 2025
Copy link
Collaborator

@felixfontein felixfontein 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 your contribution!

CC @russoz

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Feb 12, 2025
Copy link
Collaborator

@russoz russoz left a 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.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 14, 2025
@felixfontein
Copy link
Collaborator

For reference, the PR that refactored the module: #9238; the issue in which the main discussions happened: #9131.

@ansibullbot ansibullbot added integration tests/integration tests tests and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 17, 2025
@k0ste
Copy link
Author

k0ste commented Feb 17, 2025

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?

The original locale_gen, as I remember, released with Ansible 1.6. There are documentation

#!/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 when for tests. Expect tests to fail if the appropriate language packs are not installed

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 17, 2025
@russoz
Copy link
Collaborator

russoz commented Feb 17, 2025

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?

The original locale_gen, as I remember, released with Ansible 1.6. There are documentation

#!/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 when for tests. Expect tests to fail if the appropriate language packs are not installed

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 when, but as @felixfontein pointed out, the tests will need adjustments to be executed in those systems. Please do adjsut them accordingly.

@k0ste k0ste force-pushed the help branch 3 times, most recently from 933ddb4 to 452f3e3 Compare February 18, 2025 09:14
@k0ste k0ste force-pushed the help branch 3 times, most recently from 9784dfc to e6214d5 Compare February 18, 2025 10:19
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 18, 2025
@k0ste
Copy link
Author

k0ste commented Feb 18, 2025

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 when, but as @felixfontein pointed out, the tests will need adjustments to be executed in those systems. Please do adjsut them accordingly.

You are welcome. Tests added

@felixfontein felixfontein requested a review from russoz February 19, 2025 20:44
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 19, 2025
Copy link
Collaborator

@russoz russoz left a 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 the updates. I have another round of comments on the module.

# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
# SPDX-License-Identifier: GPL-3.0-or-later

- name: "Non dnf RedHat systemd are not supported"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not supported by the test or not supported by the module? If the former, please make it explicit here, so that future developers will know the tests do not cover that case. If the latter, then it should be added (possibly as a note:) to the module docs, so that users are warned not to try that combination.

Copy link
Author

Choose a reason for hiding this comment

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

c7 is retired OS, the yum also too. Is surprise, that Ansible still have tests over c7

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as ansible-core versions supported by this collection still support CentOS 7, we're kind of supporting it.

Comment on lines +30 to +37
- name: "Prepare locale.gen file for RedHat or Archlinux"
ansible.builtin.template:
src: "locale_gen.j2"
dest: "/etc/locale.gen"
owner: "root"
group: "root"
mode: "0644"
force: "yes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, this is the part where it becomes odd.

The module (again, even before my rewrite) had code to check whether a locale definition was available or not. In modern Debian/Ubuntu, it does that by checking the contents of the /usr/share/i18n/SUPPORTED file. That file is managed by the OS when packages are (un)installed.

What this task here tells me (unless I'm getting it wrong) is that RH and Arch Linux systems do not keep a list of available locale definitions. If that is so, what this task does (and possibly what your playbooks have been doing for years) is nothing more than cheating the method assert_available() into calculating that the locales we artificially write to that file (here using the template + var) are the only ones available.

If you come to think of it, it makes no sense that users were forced to create a file to make the module think that something managed by the OS was actually there (when it was not).

Having said that, I am thinking that asserting available (or not) locale definitions should be controlled by a parameter that would allow us to skip that check in the code, say skip_availability_check (False by default for backward compatibility). I understand this is a whole new thing, so it should be implemented in its own separate PR. When that happens, this step could be removed and replaced with the param skip_availability_check=True. Code-wise there's probably more to it, I am not going deep into that idea right now - as I wrote, that's a future PR.

For the time being, though, this incurs in documentation updates:

  • Not this exactly, but somewhat related, please rewrite the part of the docs that state the module only works for Debian/Ubuntu
  • Then, after mentioning it works for RH and Arch as well, please make it clear to the users that they are responsible for populating the /etc/locale.gen with the locales they want to be available to the module. Make it clear that locales not found in that file will be refuted by the module, even if their corresponding language pack packages are installed.

Copy link
Author

Choose a reason for hiding this comment

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

The module (again, even before my rewrite) had code to check whether a locale definition was available or not. In modern Debian/Ubuntu, it does that by checking the contents of the /usr/share/i18n/SUPPORTED file. That file is managed by the OS when packages are (un)installed.

The main value of the module for us is to make sure that the locales specified in the configuration actually exist. The module has helped us out more than once in this case if we forgot to install the corresponding packages.

What this task here tells me (unless I'm getting it wrong) is that RH and Arch Linux systems do not keep a list of available locale definitions. If that is so, what this task does (and possibly what your playbooks have been doing for years) is nothing more than cheating the method assert_available() into calculating that the locales we artificially write to that file (here using the template + var) are the only ones available.

In Archlinux you can:

  • use SUPPORTED file
  • install locales pregenerated (pacman -S glibc-locales)
  • prepare locale.gen file and generate locale. See documentation

In RedHat you can:

  • install glibc-all-langpacks
  • install glibc-minimal-langpack
  • glibc-langpack-<xx> for single language support

In both distro, you can list usable locales via systemd

[root@host]# localectl list-locales
C.UTF-8
ast_ES.UTF-8
en_AU.UTF-8
en_BW.UTF-8
en_CA.UTF-8
en_DK.UTF-8
en_GB.UTF-8
en_HK.UTF-8
en_IE.UTF-8
en_NZ.UTF-8
en_PH.UTF-8
en_SC.UTF-8
en_SG.UTF-8
en_US.UTF-8
en_ZA.UTF-8
en_ZW.UTF-8
pl_PL.UTF-8

If that is so, what this task does (and possibly what your playbooks have been doing for years) is nothing more than cheating the method assert_available() into calculating that the locales we artificially write to that file (here using the template + var) are the only ones available.

Definitely. Because of how the module was originally written, we simply adapted to how other distributions work. I described the meaning and value above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Arch Linux's installation instructions basically tell you to edit /etc/locale.gen and comment in the locales you want to have, and then run locale-gen (and then run echo LANG=... > /etc/locale.conf to select the default locale). (https://wiki.archlinux.org/title/Installation_guide#Localization)

Basically /etc/locale.gen already contains the list of all supported locales. There's no need to template this file, unless you want to make sure you have a specific state.

So basically I don't see why this task should run on Arch Linux.

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 20, 2025
Signed-off-by: Konstantin Shalygin <[email protected]>
@k0ste
Copy link
Author

k0ste commented Feb 20, 2025

Gentle reminder that this is a bugfix. I added all the suggested inline-changes to the code, tests, description of how administrators work with locales in different environments (nobody likes it), I also added to this PR. I hope further ideas about what to do will be transferred to some field for developers, after fixing a specific bug. Thank you

k0ste added a commit to k0ste/ansible-role-linux_helper that referenced this pull request Feb 20, 2025
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 20, 2025
- "glibc-langpack-pl"
- "glibc-langpack-en"
state: "present"
update_cache: "yes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is usually no reason why the cache should be updated in the middle of a CI run.

Suggested change
update_cache: "yes"

(Besides that, "yes" is a string, not a boolean. yes is a boolean, but a form we do not use in this collection. We always use true or false for YAML booleans.)

community.general.pacman:
name: "glibc-locales"
state: "present"
update_cache: "yes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
update_cache: "yes"

owner: "root"
group: "root"
mode: "0644"
force: "yes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
force: "yes"
force: true

Comment on lines +30 to +37
- name: "Prepare locale.gen file for RedHat or Archlinux"
ansible.builtin.template:
src: "locale_gen.j2"
dest: "/etc/locale.gen"
owner: "root"
group: "root"
mode: "0644"
force: "yes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arch Linux's installation instructions basically tell you to edit /etc/locale.gen and comment in the locales you want to have, and then run locale-gen (and then run echo LANG=... > /etc/locale.conf to select the default locale). (https://wiki.archlinux.org/title/Installation_guide#Localization)

Basically /etc/locale.gen already contains the list of all supported locales. There's no need to template this file, unless you want to make sure you have a specific state.

So basically I don't see why this task should run on Arch Linux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

locale_gen: broken in Ansible 10
4 participants