Make galaxyproject.gxadmin role install its own dependencies #11
Conversation
Add tasks that invoke the system's package manager to install all packages the role needs to run. Let said tasks be disabled via boolean role variables (they default to enabled).
tasks/main.yml
Outdated
| - name: Update apt cache (Debian derivatives) | ||
| become: true | ||
| ansible.builtin.apt: | ||
| cache_valid_time: "{{ gxadmin_apt_cache_valid_time }}" | ||
| when: >- | ||
| ansible_os_family == "Debian" | ||
| and gxadmin_manage_dependencies | ||
| and gxadmin_manage_apt_cache | ||
|
|
||
| - name: Ensure role and gxadmin dependencies are installed | ||
| become: true | ||
| ansible.builtin.package: | ||
| name: | ||
| - git | ||
| - make | ||
| state: present | ||
| when: gxadmin_manage_dependencies |
There was a problem hiding this comment.
- Why are you doing this with
aptrather than preferredpackage? - I don't love having a flag for manaing the apt cache, that's so weirdly debian specific and not relevant for "download a simple package"
There was a problem hiding this comment.
I am using ansible.builtin.apt because updating the APT cache is Debian-specific. ansible.builtin.package does not update the repository metadata.
I think we do not need to do the same for EL because DNF is smarter than APT in this regard.
I agree with (2) and would also prefer not having this thing in the role variables. Let's say whatever I did, there was a 50% chance that you would disagree, and removing lines is easier than adding them haha
I proceed to remove gxadmin_manage_apt_cache and gxadmin_apt_cache_valid_time.
There was a problem hiding this comment.
Yes, I'm aware apt cache is debian specific, and the differences between the two.
However, the point was the rationale for forcing a cache update for installing git: A cache update should only be required in cases of adding a new repository, where the packages in the newly added repo won't be known to the cache. (Or in cases of e.g. a docker container where the cache can be ancient, but this isn't relevant for 99% of users, and everyone starts their docker containers with a big 'update the cache' stanza anyway.) Since we're not adding a new apt repository, it thus shouldn't be needed.
There was a problem hiding this comment.
Well, why should this role not work on the official Ubuntu Docker image...? In general it is unknown whether the cache is ancient or not. That's why I said DNF is smarter 😅.
So we (some other role developers) avoid this; the reason being that this role now must run as root unless the user figures out they can toggle off the dependency management, so they can run as an unprivileged user which is relevant for some of our environments. That's why ansible-galaxy doesn't install system packages for you. |
Remove the `gxadmin_manage_apt_cache` and `gxadmin_apt_cache_valid_time` role variables.
|
(Just adding on further, I can definitely see an argument for both, and a rationale for it, but this package was designed in keeping with other galaxyproject packages which tend to avoid dependencies.) |
Thanks, this is the kind of feedback I was looking for. If I can think of a good alternative I will reopen this PR. |
|
So I don't think we need to close this, I'd be very happy for other admins to have input on this, maybe times are changing. That said at least we should have it default to not manage dependencies, to keep it in the same default behaviour as other galaxyproject roles. (Alternatively we can do something like |
Ok, let's keep this open to let other admins have a say as well :) |
|
Related #3. |
| @@ -1,4 +1,21 @@ | |||
| --- | |||
| - name: Update apt cache (Debian derivatives) | |||
| become: true | |||
There was a problem hiding this comment.
Probably also needs become_user: root because the role may already be running under become: true, become_user: someone_else.
|
I have thought about this for a few days, and I still think this is a bad idea and I want to close the PR. This approach simply does not scale, be it because of the different requirements or more importantly, because this discussion would need to take place for every Ansible role that exists. I think this should be managed either by Ansible itself or by role that checks what roles are imported in a play or lets you install packages for specific roles. In the latter case, a list of the packages each role depends on can be maintained in |
…rently It is a time sink and very annoying to have to re-run playbooks until no more "command not found" errors appear because of forgetting to install role dependencies (meaning system packages, not other roles). See issue galaxyproject/ansible-gxadmin#3 for an example of someone complaining about this recurring problem. I thought of creating a role called "usegalaxy_eu.packages" that alleviates this problem. The idea is to define a list of the packages each role depends on in group_vars/all.yml (see the example below), then have the role check what other roles are imported in a play and install the corresponding dependencies. ```yaml # Ansible role package dependencies (managed by usegalaxy_eu.packages) packages: usegalaxy_eu.handy.os_setup: - findutils galaxyproject.gxadmin: - git - make - "postgresql{{ '-client' if ansible_os_family == 'Debian' }}" usegalaxy-eu.bashrc: - python3-psycopg2 - python3-pyyaml ``` The idea to create this role stems just from stumbling upon the problem. See galaxyproject/ansible-gxadmin#11 for an additional discussion on the topic.
Add tasks that invoke the system's package manager to install all packages the role needs to run.
From my experience as a Galaxy admin, I would say I'd rather have the roles install the packages they need to run rather than having to install them separately the hard way by discovering which ones are missing by trial and error. After all, I am forced to install them anyways.
I have added a couple of boolean role variables to disable this behavior (for edge cases).
I think it makes sense to request a review both from @hexylena and from admins the other main Galaxy instances (@natefoo, @cat-bro) since this is an important role for all of us and I might have missed something when considering what may go wrong.
I have tested this with #10.