geerlingguy / ansible-role-php-versions

Ansible Role - PHP Versions
https://galaxy.ansible.com/geerlingguy/php-versions/
MIT License
98 stars 73 forks source link

Cannot override default packages with ubuntu #3

Closed FinBoWa closed 7 years ago

FinBoWa commented 7 years ago

We were using php_packages to override the php defauls on drupal vm and this prevents it now:

https://github.com/geerlingguy/ansible-role-php-versions/blob/master/vars/Debian.yml#L24

We did move the additional packages under php_packages_extra but took some time to figure the reason out .

With centos you could still override the defaults so this might cause questions when swapping between distros.

geerlingguy commented 7 years ago

@FinBoWa - Hmm... that seems like a bug to me :/

We should still allow the overriding of php_packages if you need an entirely different set. @oxyc do you know if there was some easy way of doing this? I thought we had it working at some point where you could override php_packages in config.yml still... but thinking back in the past 6 months, I haven't had to do that on a project, so I haven't really tested it :-O

oxyc commented 7 years ago

Yikes, I entirely forgot about that! It did work before we moved it into a separate role. The reason it worked before was that Drupal VM included {{ ansible_os_family }}.yml with the vars before any other *.config.yml files. Now that it's in a role, it's loaded after and overrides the user settings.

geerlingguy commented 7 years ago

D'oh! Well, then... maybe there's a way we could detect if it's already been overridden? Or does the PHP role's defaults already take effect by the time this role runs?

e.g. if php_packages is already set (and not by the PHP role), we could reasonably assume the upstream playbook is setting it on purpose, and then we ignore it?

oxyc commented 7 years ago

I dont believe that would work. The usual set_fact technique would detect the PHP role's variables as they are loaded statically, right?

The only way I can see to fix it is to use include_role over at Drupal VM and run it before the config files are read. This should work as this role doesn't have any dependencies.

 provisioning/playbook.yml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/provisioning/playbook.yml b/provisioning/playbook.yml
index 3f58303..5cae062 100644
--- a/provisioning/playbook.yml
+++ b/provisioning/playbook.yml
@@ -7,6 +7,10 @@
     - ../default.config.yml

   pre_tasks:
+    - include_role:
+        name: geerlingguy.php-versions
+      tags: ['php', 'xdebug', 'database']
+
     - include: tasks/config.yml
       tags: ['always']

@@ -40,7 +44,6 @@
     - { role: geerlingguy.apache, when: drupalvm_webserver == 'apache', tags: ['webserver']}
     - { role: geerlingguy.apache-php-fpm, when: drupalvm_webserver == 'apache', tags: ['webserver'] }
     - { role: geerlingguy.nginx, when: drupalvm_webserver == 'nginx', tags: ['webserver'] }
-    - { role: geerlingguy.php-versions, tags: ['php', 'xdebug', 'database'] }
     - { role: geerlingguy.php, tags: ['php'] }
     - { role: geerlingguy.php-pecl, tags: ['php'] }
     - { role: geerlingguy.composer }
oxyc commented 7 years ago

Derp just realized we depend on the php_version variable which is defined in the config files...

Edit: A nasty way to fix it would be to run config.yml twice, once before the role and once after.

oxyc commented 7 years ago

I take it back, none of these variables should actually be available as they're all OS-specific and therefore set dynamically (and not available when this role runs). I'll make a PR using set_fact.

geerlingguy commented 7 years ago

Fixed via https://github.com/geerlingguy/ansible-role-php-versions/pull/4 — tagging a new release and will pull it into Drupal VM master branch as well.