linux-system-roles / postfix

An ansible role which configures postfix
https://linux-system-roles.github.io/postfix/
GNU General Public License v3.0
13 stars 20 forks source link

Remove outdated ansible managed header and use {{ ansible_managed | comment }} #42

Closed spetrosi closed 2 years ago

spetrosi commented 2 years ago

I am testing with ansible_managed set as such in ansible.cfg:

ansible_managed = WARNING: This file is managed by ANSIBLE.
  template: {file}
  modified on: %Y-%m-%d %H:%M:%S
  by user: {uid}
  from ansiblemaster: {host}

This results in the following at /etc/postfix/main.cf:

# BEGIN ANSIBLE MANAGED BLOCK
#
# WARNING: This file is managed by ANSIBLE.
# template: /home/spetrosi/Documents/work/linux-system-roles/postfix/tests/roles/linux-system-roles.postfix/templates/get_ansible_managed.j2
# modified on: 2022-02-17 16:15:39
# by user: spetrosi
# from ansiblemaster: fedora
#
# END ANSIBLE MANAGED BLOCK

It is dynamic because it uses date, which means that it is never idempotent. This is, however, how the upstream reporter of the ansible_managed issue in https://github.com/linux-system-roles/timesync/issues/123 used it. I came up with blockinfile because lineinfile is not able to replace the existing multi-line ansible_managed block, it just pastes another one above.

spetrosi commented 2 years ago

@richm I fixed the redundant trim. That was a very good catch, thank you.

spetrosi commented 2 years ago

@nhosoi I guess we do not need tests for ansible_managed, am I right? I looked at the previous ansible_managed PRs and did not find ones similar to what I did here. Other PRs either put ansible_managed in templates, or via Ansible modules. Please correct me if I am wrong.

spetrosi commented 2 years ago

I noticed a bug with this solution, if I put time into the ansible_managed definition in ansible.cfg, the role puts the current time stamp. So the header in the file is as is but does not update with the further role runs. I'll try to think what to do with this, please suggest your ideas if any too :)

[root@ci-vm-10-0-138-91 ~]# head /etc/postfix/main.cf
# BEGIN ANSIBLE MANAGED BLOCK
#
# WARNING: This file is managed by ANSIBLE.
# template: get_ansible_managed.j2
# modified on: 2022-02-21 09:03:49
# by user: spetrosi
# from ansiblemaster: fedora
#
# END ANSIBLE MANAGED BLOCK
# Global Postfix configuration file. This file lists only a subset
nhosoi commented 2 years ago

I noticed a bug with this solution, if I put time into the ansible_managed definition in ansible.cfg, the role puts the current time stamp. So the header in the file is as is but does not update with the further role runs. I'll try to think what to do with this, please suggest your ideas if any too :)

Ahhh, it's so tricky, isn't it? The issue is shared among multiple roles now... If the timestamp format is fixed like %Y-%m-%d %H:%M:%S, we could just compare the config files after eliminating the timestamp part and learn if the rest of the contents are identical or not. But the timestamp format is quite flexible. I'm not sure if there's a perfect way to delete just the timestamp from the header... :(

richm commented 2 years ago

This is why it is not a good practice to put a timestamp in the ansible_managed header - https://github.com/redhat-cop/automation-good-practices/blob/main/roles/README.adoc#generating-files-from-templates

When commenting, don’t include anything like "Last modified: {{ date }}". This would change the file at every application of the role, even if it doesn’t need to be changed for other reasons, and thus break proper change reporting.
richm commented 2 years ago

So if the user wants ansible_managed to include a timestamp, then they will have to know that they are breaking role idempotency and check_mode - we should not do anything to prevent this in the roles, we should just suggest that this is not recommended practice because it breaks idempotency and check_mode

nhosoi commented 2 years ago

This is why it is not a good practice to put a timestamp in the ansible_managed header - https://github.com/redhat-cop/automation-good-practices/blob/main/roles/README.adoc#generating-files-from-templates

When commenting, don’t include anything like "Last modified: {{ date }}". This would change the file at every application of the role, even if it doesn’t need to be changed for other reasons, and thus break proper change reporting.

So, can we refer the good practice somewhere in the rhel-/linux-system-roles doc and leave the ansible_managed code as it is?

spetrosi commented 2 years ago

I must have been unclear, I am not talking about idempotency, it's fine that idempotency doesn't work with timestamps in ansible_managed, I do not think that we should mention it as it's a common Ansible knowledge.

The issue is that time is displayed incorrectly. I am running the following task against 1minutetip:

- debug:
    var: __postfix_ansible_managed
  vars:
    __postfix_ansible_managed: "{{
      lookup('template', 'get_ansible_managed.j2') }}"

I run it yesterday the first time, since then the output shows the same result with the same timestamp:

TASK [linux-system-roles.postfix : debug] ****************************************************************************************************************************************************
ok: [10.0.139.83] => {
    "__postfix_ansible_managed": "#\n# WARNING: This file is managed by ANSIBLE.\n# template: get_ansible_managed.j2\n# modified on: 2022-02-21 09:03:49\n# by user: spetrosi\n# from ansiblemaster: fedora\n#\n"

I expect the timestamp to change each time I am running the playbook, but it does not.

spetrosi commented 2 years ago

After I reboot my laptop, I see the same 2022-02-21 09:03:49 timestamp on the output... I think I can merge this PR now and deal with this weird issue after because the role works correctly the first time.

spetrosi commented 2 years ago

Did s/__postfix_ansible_managed/__lsr_ansible_managed for consistency with other lsr roles and rebased. Merging now, thank you for your reviews.