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

Use distribution version as default when major version isn't defined #425

Closed
wants to merge 1 commit into from

Conversation

jbronn
Copy link
Contributor

@jbronn jbronn commented Mar 18, 2021

OpenBSD doesn't have an ansible_facts.distribution_major_version fact; I think SmartOS is the same way, but haven't confirmed. For example, here are all the ansible_distribution* facts on OpenBSD:

{
  "ansible_distribution": "OpenBSD",
  "ansible_distribution_release": "release",
  "ansible_distribution_version": "6.8"
}

In this PR I've changed the distribution version to be used as a fallback default when trying to load variables. Otherwise, on these platforms Ansible will fail with a cryptic message about first_found lookup failing (because it can't template its arguments).

… defined.

Signed-off-by: Justin Bronn <jbronn@gmail.com>
@schurzi schurzi self-assigned this Mar 23, 2021
@schurzi
Copy link
Contributor

schurzi commented Mar 23, 2021

Nice catch, unfortunately we currently have no CI testing on OpenBSD so this didn't come up sooner.

However I have some problems with your patch. Using the fact ansible_facts.distribution_version is plainly wrong for all other use-cases we want to cover here. :(

To make this clear, we use this task to load OS specific variables. In this particular case we have different variables for RHEL and RHEL8 distributions. I know, that your logic will not really disturb this, but it will make it very hard to reason about which files are loaded.

I'm thinking about some better solution, for the time being, could you try to "fake" this fact via an Ansible task in your playbook:

    - name: fake distribution_major_version fact
      set_fact:
        ansible_facts: "{{ ansible_facts | combine({'distribution_major_version':ansible_facts.distribution_version}) }}"
    - name: include role
      include_role:
        name: ssh_hardening

@schurzi
Copy link
Contributor

schurzi commented Mar 23, 2021

now I had a talk with @rndmh3ro and we both agreed, that we don't want to complicate the os variable selection any further. And we both also agreed, that inserting the above task to "fake" the ansible_major_version fact in the ssh_hardening role might be a good workaround. But we should only do that for a very limited number of operatingsystems, namely OpenBSD and SmartOS.

@jbronn are you up to do that?

@schurzi
Copy link
Contributor

schurzi commented Feb 25, 2023

sorry to let this slip, but a different solution was already merged with #597

@schurzi schurzi closed this Feb 25, 2023
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