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

feat(debian) enable module to work on debian #6

Merged
merged 5 commits into from
Feb 6, 2019

Conversation

ties
Copy link
Contributor

@ties ties commented Jan 28, 2019

Add support for Debian based on the documentation in debian wiki
and discussion in #5.

Condition for apt cache refresh ("only if Ubuntu") was removed since
the file is only included on Ubuntu.

Add support for Debian based on the documentation in debian wiki
and discussion in [0].

[0]: githubixx#5
state: present
update_cache: yes
run_once: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied this from the issue #5. Do not think it is needed. Could fix this and squash if you prefer this

tasks/main.yml Outdated Show resolved Hide resolved
@ties ties force-pushed the feature/debian_install branch 2 times, most recently from 224ed30 to 0289003 Compare January 30, 2019 17:36
There is no equivalent package of linux-headers-generic on debian.
Package installation needs to specify the architecture (i.e. amd64),
which is captured from dpkg output.
Before Archlinux was split out using ansible_os_family. But since
ansible_os_family overlaps for Debian and Ubuntu, two when
statements were used to split out these cases:

  - All arch derivations
  - Debian
  - Ubuntu

New style is cleaner. Arch derivations can still be used by
overiding ansible_distribution in inventory.
Copy link
Owner

@githubixx githubixx left a comment

Choose a reason for hiding this comment

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

Basically LGTM. Some small changes. See inline comments.

files/etc/apt/preferences.d/limit-unstable Outdated Show resolved Hide resolved
tasks/setup-debian.yml Outdated Show resolved Hide resolved
tasks/setup-debian.yml Show resolved Hide resolved
tasks/setup-debian.yml Show resolved Hide resolved
@ties
Copy link
Contributor Author

ties commented Feb 5, 2019

I (just) tested this role locally using a set-up with 3 clean vagrant virtual machines and the VPN came up as expected 🙂.

- name: Install kernel headers to compile wireguard with DKMS
apt:
name:
- "linux-headers-{{ dpkg_arch.stdout }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a funny thing happening here by the way. In the ansible -m setup localhost output there is facter_os.architecture which contains amd64.

In ansible_architecture this is normalized to 'x86_64' and not usable.

Copy link
Owner

Choose a reason for hiding this comment

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

I can find this variables:

    "ansible_architecture": "x86_64",
    "ansible_machine": "x86_64",
    "ansible_userspace_architecture": "x86_64",

But none with amd64. But at least it is consistent and AFAIK it's also the official declaration for a x86 64bit CPU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x86_64 is the consistent way of describing the architecture. On some machines I see the following - but definitely out of scope for this change. Possibly good for the dkms-headers role:

# ansible -m setup localhost | sed -e 's/.* =>//g' | jq '.ansible_facts.facter_os'
{
  "architecture": "amd64",
  "distro": {
    "codename": "buster",
    "description": "Debian GNU/Linux buster/sid",
    "id": "Debian",
    "release": {
      "full": "testing",
      "major": "testing"
    }
  },
  "family": "Debian",
  "hardware": "x86_64",
  "name": "Debian",
  "release": {
    "full": "buster/sid",
    "major": "buster/sid"
  },
  "selinux": {
    "enabled": false
  }
}

@ties
Copy link
Contributor Author

ties commented Feb 6, 2019

I tried to use facter_os['architecture'] instead of the output of the dpkg command the branch runs but can not get it to work unfortunately from the tasks. The (seemingly derived) ansible_architecture variable is not usabe (x86_64 instead of amd64).

I do not know if facter is always available when running ansible. The ansible_os data does not seem to be available on my laptop (Fedora) and on a Ubuntu machine.

@githubixx
Copy link
Owner

I'll merge this now. Thx for your contribution!

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.

None yet

2 participants