saltstack-formulas / nginx-formula

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

Included snippets, servers & certs for passenger #246

Closed ErisDS closed 5 years ago

ErisDS commented 5 years ago

As it stands, including nginx.passenger doesn't do enough to result in a working install, unlike nginx. In my usecase, we want to be able to swap between nginx and nginx.passenger and have them be interchangeable with the same behaviour, bar the additional install & config behaviour for passenger.

IMO, that could be structured in a few ways:

  1. Reuse the nginx state:

Including the state nginx with install_from_phusionpassenger: true in your pillar installs passenger + does everything that the nginx state does - if you only include nginx.passenger, you only get the passenger install same as now

  1. Addon:

Treat the passenger state as an addon - so in top.sls you define:

Buuut in that case I think order is confusing and going to cause problems.

  1. Simple: just update passenger to work the same way as nginx (as in this PR)

I'm using this on my fork.

myii commented 5 years ago

@ErisDS Thanks for this PR. As mentioned on Slack, commit messages are significant for semantic-release. There's more info in the CONTRIBUTING document.

As a suggestion, the first line of the commit message could be changed to:

feat(passenger): include snippets, servers & certs for passenger 

The rest of the lines can stay as they are.

ErisDS commented 5 years ago

Option 4:

Any one using the old behaviour can get it back by changing nginx.passenger to nginx.passenger-pkg. New comers get the same behaviour regardless of if they include nginx or passenger.

Happy to work on any one of these options, if there is consensus as to the preferred approach.

aboe76 commented 5 years ago

@ErisDS I don't see a problem with the approach you took. With number 3: Simple: just update passenger to work the same way as nginx (as in this PR)

Most people who used this as an add-on, will already have nginx configured in pillar. So duplicating the states won't do any harm.

saltstack-formulas-travis commented 5 years ago

:tada: This PR is included in version 2.3.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: