-
-
Notifications
You must be signed in to change notification settings - Fork 139
Conversation
Note that I also tried updating |
tasks/setup-Debian.yml
Outdated
apt: | ||
name: gcc | ||
state: present | ||
update_cache: yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically let playbooks set update_cache
in their own pre_tasks rather than in a role itself. I would rather not add this parameter here for consistency's sake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a better solution is to remove this step entirely. As I said at the top, this is the only step that is specific to Debian. Subject to testing/debugging.
tasks/setup-Debian.yml
Outdated
update_cache: yes | ||
become: yes | ||
|
||
- name: Check if homebrew grandparent directory is already in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically 'parent' (not grandparent) according to the registered var name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More context:
- name: Check if homebrew grandparent directory is already in place.
stat:
path: "{{ homebrew_prefix | dirname }}"
register: homebrew_prefix_parent
check_mode: no
In defaults/main.yml
we have
homebrew_install_path: "{{ homebrew_prefix }}/Homebrew"
My thinking is that
homebrew_install_path
is the "homebrew directory" (more precisely the "homebrew install directory")homebrew_prefix
is the "homebrew parent directory"homebrew_prefix | dirname
is the "homebrew grandparent directory"
so I do not think we want to replace "grandparent" with "parent" here. If you think this is confusing, then we can come up with something better.
Or maybe I should just change homebrew_prefix_parent
to homebrew_grandparent
?
tasks/main.yml
Outdated
mode: 0775 | ||
become: yes | ||
when: "ansible_distribution_version is version('10.13', '<')" | ||
- include_tasks: setup-Debian.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work on other Linux flavors too, right? For example, CentOS or Ubuntu?
Ideally, I want to get tests added for those platforms (though that requires restructuring the Travis CI tests a little so I can use base images like I do for my other roles like geerlingguy.apache
for linux, versus Travis CI's macOS execution environments that I currently do, depending on the test matrix).
But doing this (naming the setup tasks Debian
means that it wouldn't work for any other distros besides Debian (and I guess Ubuntu since it's keying on ansible_os_family
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below.
meta/main.yml
Outdated
company: "Midwestern Mac, LLC" | ||
license: "license (BSD, MIT)" | ||
min_ansible_version: 2.5 | ||
platforms: | ||
- name: Debian |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also work on Ubuntu
the way it's currently structured. And with a couple tweaks mentioned elsewhere in this review, it should also work on EL
(Red Hat / CentOS) and Fedora
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I assumed that the platforms
in the meta file were the same as ansible_os_family
.
See below.
I will update this PR later this week (probably Thursday). The first two comments look simple enough. (Famous last words. ;) As for other Linux distributions, how much manual testing do you expect? Or should we delay this PR until we have some automated testing? I will also review the What is your preferred structure for including generic-Linux steps? |
@benjifisher - Maybe have a condition for if os family != Darwin, include linux tasks. Also, no need to have the testing as part of this PR. I can work on that in a follow-on issue (it could be slightly strange since I'll be mixing two testing paradigms). |
This should normally be done in pre_tasks.
Instead, do it under pre_tasks in the sample playbook. Removing this step makes the playbook work on more Linux distributions.
A few of the line comments dealt with relaxing the Debian restriction. As I said at the top, one part of making this work in other Linux distributions is to remove the I tested this with VirtualBox/Vagrant. When I first made this PR, I tested Ubuntu 18.04. Today, I tested with
---
- hosts: vagrant
pre_tasks:
- name: Ensure that requirements are available.
yum:
name:
- gcc
- git
state: present
update_cache: yes
become: yes
when: ansible_os_family == 'RedHat'
roles:
- geerlingguy.homebrew and the command
|
I added a link to https://docs.brew.sh/Homebrew-on-Linux#linuxwsl-requirements in Linux/WSL Requirements
Debian or Ubuntu
Fedora, CentOS, or Red Hat
Is the link good enough or should we add some of this information to |
Closing all issues in this repository in preparation for a migration to |
Homebrew is now available for Linux: see https://docs.brew.sh/Homebrew-on-Linux
There are some differences:
/home/linuxbrew/.linuxbrew/Homebrew
instead of/usr/local/Homebrew
.brew cask
.brew
is not installed in/usr/local/bin/
, you need an extra step (modifyPATH
or symlink to/usr/local/bin/brew
, for example) to use it conveniently.I tried adding
when: ansible_os_family == 'Darwin'
tometa/main.yml
, following Ansible: Conditional role dependencies, but that did not work. I guess I should update theREADME
to recommend using theosx-command-line-tools
role.I am not sure whether to install
gcc
(as the role currently does) or leave that to the documentation. If we leave it to the documentation, then this should work without further changes on a wide variety of Linux distributions.I tested this on Ubuntu 18.04 using Vagrant. I also tested on
localhost
(Pop!_OS 19.10) but it failed on the task of installinggcc
with thepython3
version of the error message reported at ansible/ansible#14468. I tried to debug that without success.