stuvusIT / ansible_systemd_timesyncd

Ansible role to set timezone and configure systemd-timesyncd
Other
18 stars 23 forks source link

Install systemd-timesyncd before configuring it - the correct way #17

Closed Lycowolf closed 1 year ago

Lycowolf commented 2 years ago

The change introduced in dac573c was wrong:

The correct way to do this is to just use the system package manager to install the package unconditionally.

This PR reverts the errorneous commit and fixes the original issue.

chrisbra commented 2 years ago

FWIW: I just run into this problem and applied this change. I can confirm this change/revert works

neumantm commented 2 years ago

You are correct, my attempt to fix the issue was not very good. Thank you for contributing a better alternative. I will try it out on our systems and check that it doesn't cause problems.

neumantm commented 2 years ago

I have the following problem with this fix: When running the task on a debian buster i get the following error (because no package with that name exists):

TASK [systemd-timesyncd : Install systemd-timesyncd] ************************************************
fatal: [smtp01]: FAILED! => {
    "changed": false
}

MSG:

No package matching 'systemd-timesyncd' is available

Do you have any suggestion on how to prevent this with your proposal?

Lycowolf commented 2 years ago

Hi,

the common idiom to just know the right package name using conditional set_factsor by including a vars file whose name is based on the distro, and then unconditionally install it.

In Debian's case, it was part of systemd package until buster and then they moved it to systemd-timesyncd (for all architectures except MIPS, WTF?). I think something along the lines of

- set_fact: 
    pkg_name: "{{ 'systemd' if (ansible_distribution == 'Debian' and ansible_distribution_major_version < 10) else 'systemd-timesyncd' }}"
- package:
    name: "{{ pkg_name }}"
    state: present

would be good enough here.

Also, I am sorry I won't update this PR today nor tomorrow, as we have holidays in our country and I have a lot of different work before me at my home. I set the PR to be editable by the upstream maintainer: you should be able to edit it yourself as needed.

EDIT: better pseudocode

neumantm commented 2 years ago

I added a new change based on the suggestion in https://github.com/stuvusIT/systemd-timesyncd/pull/17#issuecomment-1175166125. Is everybody happy with the current version? @haslersn as this now contains changes from me, can you review this?

neumantm commented 2 years ago

Closes #14, #18

haslersn commented 2 years ago

I don't think there's any case, where we actually want to install systemd. Especially if the README states that systemd is a prerequisite. So @Lycowolf's idiom doesn't really make sense in our special case here.

Maybe install the package with

  when: ansible_distribution != 'Debian' or ansible_distribution_major_version is version('10', '>')
neumantm commented 1 year ago

Superseded by #19.