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

Fix hardcoded variables on including role repos #278

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

mkayontour
Copy link
Member

@mkayontour mkayontour commented Mar 26, 2024

This was a intial idea to reuse the role repos for enabling the epel release. But if included it will deactivate the release repos and if not set to false. It will enable the repos even if you just want the epel release.

So I will remove it and will install epel-release instead.

@Donien can you give your thoughts if this is the right way?

ref #270

@cla-bot cla-bot bot added the cla/signed label Mar 26, 2024
@Donien
Copy link
Collaborator

Donien commented Mar 26, 2024

TLDR

I think your proposed solution is the most suitable. It is readable, does the job and is still bound to a conditional.

And since the repos role doesn't ever actually remove EPEL (I believe) no cycle of changes should ever occur.


Extra thoughts

Inside the repos role icinga_repo_epel: false is the default value. Meaning, unless somebody wants to use EPEL, EPEL will not be activated.
If somebody uses both roles, repos and monitoring_plugins, they should match / try to do the same thing.

We could change the task as follows:

- name: Activate epel repository
  include_role:
    name: icinga.icinga.repos
  when: icinga_monitoring_plugins_epel | bool if icinga_repo_epel is not defined else icinga_repo_epel | bool

This would mean that the repos role's variable icinga_repo_epel takes precedence. Variables concerning other repositories (e.g. Icinga Testing) simply won't be stated explicitly as they might already be part of the play.

And here comes the problem: When including both roles within the same play this should work fine. When including multiple roles, at least their variables from default/main.yml are loaded prior to executing the first task of the first role to be called.
Thus, icinga_repo_epel is always set, either by vars in the play or by default/main.yml. The same is true for the other variables.

But: If using different plays, one for each role, icinga_repo_testing might be true in one play and false in the next one.


Possible Solutions

Reuse Role

Like this:

- name: Activate epel repository
  include_role:
    name: icinga.icinga.repos
  when: icinga_monitoring_plugins_epel | bool if icinga_repo_epel is not defined else icinga_repo_epel | bool

icinga_monitoring_plugins_epel: true in combination with icinga_repo_epel: false will still skip this task. If icinga_repo_epel is not defined (e.g. only using role monitoring_plugins), then icinga_monitoring_plugins_epel will be evaluated.

Using both roles in different plays could still lead to a cycle of changes. So this is not really clean.

Simply activate EPEL

We could reuse the repos role. But as your proposed fix shows, we don't gain much by that.

- name: Activate epel repository
  ansible.builtin.yum:
    name: epel-release
    state: present
  when: icinga_monitoring_plugins_epel|bool

Don't handle EPEL

We could simply not handle EPEL in role monitoring_plugins. We already provide a way to enable EPEL. We could let the role fail (and perhaps tell them that it might be due to EPEL missing).

Of course users would have to use the role repos even if they only wanted to use monitoring_plugins though.

Just a thought.

@mkayontour
Copy link
Member Author

The real issue is that, the repos role will activate icinga_repo_stable per default, which is a unwanted sideeffect.
And when we disable it - you can't install anything icinga2 related afterwards because the repo is not temporarily disabled.

Thanks, I'll merge and close the issue.

@mkayontour mkayontour merged commit 869cf11 into main Mar 27, 2024
1 check passed
Donien pushed a commit to Donien/ansible-collection-icinga that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants