saltstack-formulas / nginx-formula

Nginx Salt Formula
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
163 stars 420 forks source link

install_from_ppa behavior checks the os_family grain against "Debian", then uses an ubuntu-specific repo #49

Closed teepark closed 10 years ago

teepark commented 10 years ago

https://github.com/saltstack-formulas/nginx-formula/blob/1f5d6bb7ee1973a088502185e9b8718cc7a839bb/nginx/package.sls#L39

this sticks in an invalid apt repo on debian, and (at least on wheezy) you end up with nginx 1.2

gravyboat commented 10 years ago

That's odd, are you able to compare the repo it's dropping in, versus what the actual repo should be?

teepark commented 10 years ago

https://github.com/saltstack-formulas/nginx-formula/blob/1f5d6bb7ee1973a088502185e9b8718cc7a839bb/nginx/package.sls#L48

on my debian 7 image it inserts deb http://ppa.launchpad.net/nginx/stable/ubuntu wheezy main, which doesn't exist. the whole launchpad PPA system is ubuntu-specific.

based on http://wiki.nginx.org/Install, for debian we should point straight to nginx.org's own apt repo.

gravyboat commented 10 years ago

@teepark Alright that should probably be changed then, if you want to make a PR to change that up it would be awesome!

teepark commented 10 years ago

@gravyboat what do you think about using nginx.org in the case of debian or ubuntu, and making the use_ppa pillar boolean override to the ppa in the ubuntu case?

the default repos for debian and ubuntu are both pretty far behind.

gravyboat commented 10 years ago

@teepark I think that should be an option users could configure, so you could have a pillar value to use the nginx.org repos, This would also reduce the number of checks (note this is just a draft, might be some errors):

{% if salt['pillar.get']('nginx:nginx_repo') == 'True' or salt['grains.get']('os') == 'Debian' %}
.... (official repo setup)
{% elif salt['grains.get']('os') == 'Ubuntu' %}
.... (ppa setup)
{% endif %}

This might make it a bit complicated, but I think it could be paired down.

teepark commented 10 years ago

something got lost in the formatting there, but I think that approach would give people the PPA even if they didn't ask for it when they're on Ubuntu?

since we now have more than 2 choices, what about making the pillar a string with multiple recognized options?

{% set repo_source = pillar.get('nginx', {}).get('repo_source', 'default') %}
{% if repo_source == 'ppa' and grains.get('os') == 'Ubuntu' %}
... PPA setup, disable official repo
{% elif repo_source == 'official' and grains.get('os') in ('Ubuntu', 'Debian') %}
... official repo setup, disable PPA
{% else %}
... explicitly disable both custom repos, let the distro repo provide it
{% endif %}
gravyboat commented 10 years ago

Hmm, that could definitely work.