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 11 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 6 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

@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
k0ste and others added 10 commits February 21, 2025 12:11
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>
@k0ste k0ste force-pushed the help branch 2 times, most recently from 54019cf to 1f5695e Compare February 21, 2025 09:16
@ansibullbot
Copy link
Collaborator

@k0ste This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>
@k0ste
Copy link
Author

k0ste commented Feb 21, 2025

Removed preparation tasks for Archlinux. Now somebody need to take a look what with container glibc?

@felixfontein
Copy link
Collaborator

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.

@felixfontein
Copy link
Collaborator

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).

@felixfontein
Copy link
Collaborator

The implementation should [...] use localectl list-locales for any systemd distro in present days IMO

This statement is also not correct. localectl list-locales lists the installed locales. But this module is actually doing the installing on systems that use locale-gen. That's the whole purpose of the module. So localectl list-locales cannot be used by the module to figure out which locales can be installed.

@k0ste
Copy link
Author

k0ste commented Feb 22, 2025

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).

The module does, like said before, assert_avail for the locale. I re-implemented the locale tasks (more complex with distro logic) at this week, for possibility work with c9s+ and Ansible 11 k0ste/ansible-role-linux_helper@eadbd25

@russoz
Copy link
Collaborator

russoz commented Feb 23, 2025

The mechanism it uses to determine availability is to check in the file, in this case /etc/locale.gen if there is a line with that locale, either commented out or not, the key thing being the comment. If the locale is commented out, then it is still available, but not enabled. If it is not commented out (like in the template here), the module understands the locale is available and already enabled, therefore it doesn't need to do anything, therefore it becomes a no-op.

@felixfontein
Copy link
Collaborator

It would probably be helpful to have a separate module community.general.locale_info to determine which locales are available (with locale -a or localectl list-locales). But community.general.locale_gen should not be abused for that, it's really only meant for systems supporting locale-gen (which does not cover RHEL/CentOS/Fedora and friends).

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