search

saltstack-formulas / nginx-formula

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

Basic OpenResty support #268

Open faudebert opened 4 years ago

faudebert commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

This PR relies on #267 which should be thus considered before. In this PR only introduce the last 2 commits.

Describe the changes you're proposing

This pull-request attempts to reintroduce (basic) OpenResty support within the NGINX formula.

Pillar / config required to test the proposed changes

Set nginx:install_from_openresty: True and use the formula as usual.

Debug log showing how the proposed changes work

Having set nginx:install_from_openresty: True, check apply.log to see salt-call output on a clean Debian Stretch host.

Documentation checklist

Testing checklist

Additional context

I don't know why previous OpenResty support was dropped. I hope this might be of any interest.

pull-assistant[bot] commented 4 years ago
Score: 0.90

Best reviewed: commit by commit

Optimal code review plan (1 warning)

refactor(import): uniformize map.jinja imports
> `nginx/certificates.sls` 50% changes removed in refactor(pillar): na...
     refactor(certs): use jinja.map to get pillars      refactor(meta-state): use relative includes      refactor(pillar): namespace defaults to meta-state name      refactor(pillar): store defaults into yaml files      refactor(pillar): factorize some defaults      feat(openresty): add OpenResty variant      feat(openresty): add sane defaults for Debian

Powered by Pull Assistant. Last update 4edd99f ... f455b72. Read the comment docs.

faudebert commented 4 years ago

LGTM. Separate PRs would be easier to eyeball, but if Travis CI is passing should be good to go

Thanks for your feedback.

Tried to follow the guideline regarding having two distinct refactor & feat pull-requests. I've created #267 which only contains refactor changes but current feat PR relies on it. Maybe having this pull-request base pointing at #267 branch would be clearer?

noelmcloughlin commented 4 years ago

I was just passing by (I don't maintain this repo) but yes, maybe I should have reviewed #267 first. Anyway LGTM.

faudebert commented 4 years ago

Rebased on opendatasoft:upstream/generic-namespaces from https://github.com/saltstack-formulas/nginx-formula/pull/267.

myii commented 4 years ago

@sticky-note Would you mind looking at this and #267? Please let us know if time is short and we'll look at some way of getting these reviewed.

@faudebert From a reviewing perspective, smaller PRs are much easier to review, so usually get done much quicker. Even single-line changes can have significant impacts, the further we extend, the more cautious we get. We'll probably have to do that here where we leave these two PRs in place and then work through the proposals in smaller blocks in further PRs.

For example, we've already got a new standard emerging for map.jinja that has been implemented by @baby-gnu in these formulas:

We've had quite a few discussions about this in the Formulas' working group meetings, such as here:

We're also implementing a new verification standard to use before refactoring any map.jinja, which involves using InSpec testing to make sure the map is produced the same before and after:

It would be important to bring those in here first before refactoring map.jinja.

These are just some points to help you see where we are currently. Please feel free to continue the discussion. We value your contribution and feedback, it just requires a structured approach to review.

daks commented 4 years ago

I'll look at this one once #267 is merged