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

pkg-debian: fix repo idempotence #250

Closed
wants to merge 1 commit into from

Conversation

jharley
Copy link
Contributor

@jharley jharley commented Jan 17, 2020

While doing development with Molecule on 18.04, I discovered that pkg-debian.yml was written in such a way that runs could never be idempotent. This changes the repository file name logic to basically match what is happening in pkg-redhat.yml and makes the role (more?) idempotent.

Copy link
Contributor

@KSerrania KSerrania 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 the PR! I left a (pretty big) comment discussing how to make the role idempotent while keeping our current guarantees on the role behavior. WDYT?

@@ -53,7 +53,7 @@

- name: (Custom) Ensure Datadog repository is up-to-date
apt_repository:
filename: ansible_datadog_agent
filename: ansible_datadog_custom
Copy link
Contributor

@KSerrania KSerrania Feb 3, 2020

Choose a reason for hiding this comment

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

We tried doing that before, but this method has a problem in the following situation:

  • Let's say you do a first run with the following configuration:
datadog_agent_major_version: 7
datadog_apt_repo: "deb https://my.custom.repo stable main"

Then, after this run, you have:

$ find /etc/apt/sources.list.d/ -type f -print -exec cat {} \;
/etc/apt/sources.list.d/ansible_datadog_custom.list
deb https://my.custom.repo stable main
  • Then, on the following run, you decide to use an official repo and use the following configuration:
datadog_agent_major_version: 7

Because of this, the(Custom) task will be skipped, so the custom repository won't be added, but it won't be removed either, and you'll end up with:

$ find /etc/apt/sources.list.d/ -type f -print -exec cat {} \;
/etc/apt/sources.list.d/ansible_datadog_7.list
deb https://apt.datadoghq.com/ stable 7
/etc/apt/sources.list.d/ansible_datadog_custom.list
deb https://my.custom.repo stable main

and in this situation, the role may have an unexpected behavior as you can't know in advance from which source the installed package will come from.


Another problematic situation is the following:

  • First run with datadog_apt_repo: "deb https://my.custom.repo stable main"
  • Second run with datadog_apt_repo: "deb https://my.custom.repo2 stable main"
    Then you end up with:
$ find /etc/apt/sources.list.d/ -type f -print -exec cat {} \;
/etc/apt/sources.list.d/ansible_datadog_custom.list
deb https://my.custom.repo stable main
deb https://my.custom.repo2 stable main

In this case, you don't know from which custom repository will the package come from.


I think the following fix would work for both scenarios while keeping the role idempotency:

  • keep your current changes
  • remove lines 26-29 (they're useless now)
  • add the following tasks before this one:
- name: (Custom) Initialize custom repo file deletion flag to False
  set_fact:
    datadog_remove_custom_repo_file: "False"

- name: (Custom) Check if custom repository file exists
  stat:
    path: /etc/apt/sources.list.d/ansible_datadog_custom.list
  register: datadog_custom_repo_file

- name: (Custom) Flag custom repository file for deletion if different from current repository config
  set_fact:
    datadog_remove_custom_repo_file: "{{ datadog_repo_file_contents != datadog_apt_repo }}"
  vars:
    datadog_repo_file_contents: "{{ lookup('file', '/etc/apt/sources.list.d/ansible_datadog_custom.list') }}"
  when: datadog_custom_repo_file.stat.exists

- name: (Custom) Remove Datadog custom repository file when not set or updated
  file:
    path: /etc/apt/sources.list.d/ansible_datadog_custom.list
    state: absent
  vars:
    repo_file_contents: "{{ lookup('file', '/etc/apt/sources.list.d/ansible_datadog_custom.list') }}"
  when: (not (datadog_apt_repo | length > 0) or datadog_remove_custom_repo_file)  and (not ansible_check_mode)

That way, we make sure the previous custom repository is removed when:

  • datadog_apt_repo is not set (that way, we don't keep a previous custom repo activated when we want the official repos)
  • datadog_apt_repo has been modified (that way, we only use the custom repository currently specified in the role config).

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KSerrania thanks looking at my PR and for the thoughtful review! I hadn't considered the case that you outlined and agree that your proposed solution seems like the most straightforward way to address things 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

@jharley I'll cherry-pick the changes you made here and add my own then, if you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KSerrania Not at all! Thank you 🙏

@jharley
Copy link
Contributor Author

jharley commented Feb 12, 2020

Closing in favour of #262

@jharley jharley closed this Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants