[php/ng] support the use of a list of php versions by arthurzenika · Pull Request #167 · saltstack-formulas/php-formula

arthurzenika · 2019-03-26T17:32:38Z

Related to #138

arthurzenika · 2019-04-05T08:26:57Z

the approach here is to "post-process the map dictionnary" when the php version is a list

arthurzenika · 2019-04-05T08:14:03Z

php/ng/ini.jinja

    - template: jinja
    - context:
-        config: {{ serialize(settings) }}
+        config: {{ serialize(odict(settings)) }}


not sure this is needed

arthurzenika · 2019-04-05T08:21:57Z

php/ng/fpm/config.sls

+      {% endif %}
+    {% endfor %}
+    {% do conf_settings.global.update({'pid': '/var/run/php' + version + '-fpm.pid' }) %}
+    {% do conf_settings.global.update({'error_log': '/var/log/php' + version + '-fpm.log' }) %}


not very satisfied with this part, I would want the loop above to be more generic but for some reason I couldn't get it to change the subdictionnary

php/ng/fpm/config.sls

arthurzenika · 2019-04-05T15:29:36Z

php/ng/fpm/init.sls

  php_fpm_service:
    service:
      - watch:
+    {% if pillar_php_ng_version is iterable and pillar_php_ng_version is not string %}


now I'm not sure how this handles when 7.3 as a float is used. Is setting the version without quotes supported by the existing version of the formula ?

sschne · 2019-04-12T14:11:43Z

I currently see the issue, that you cannot remove the default www.conf with multi version use.
In single version use, you could add a php:ng:fpm:pools:www.conf:enabled: False and it would remove the default pool for you.
Since we use a dict for pools, with the file name as key, you cannot use that trick to remove the default pool for both versions and end up with orphaned www.conf files. How to resolve that?

arthurzenika · 2019-04-17T07:30:32Z

@pather87 can you explain your use case so I can test it out ?

sschne · 2019-04-30T12:48:39Z

To remove the default www.conf pool, you can add:

php:
  ng:
    fpm:
      pools:
        'www.conf':
          enabled: False

With the multiphp extension, this is not possible anymore, since to remove both pools of each version:

php:
  ng:
    version:
    - "7.2"
    - "7.3"
    fpm:
      pools:
        'www.conf':
          phpversion: "7.2"
          enabled: False
        'www.conf':
          phpversion: "7.3"
          enabled: False

This is obviously a duplicate key error. Maybe should add a switch in pillars to remove the default pool of both PHP versions. I will provide a patch.

n-rodriguez · 2019-07-01T01:22:19Z

I've just tested it (in production) : great work 👍

arthurzenika · 2019-07-10T09:11:15Z

@n-rodriguez great news, let's try to get this merged. @philpep should take a look at this merge request today to give us an extra opinion.

myii · 2019-07-10T13:04:34Z

@arthurlogilab Since starting this PR, this formula has been updated to use semantic-release. In order to benefit from that upon merge, the commit messages need to be updated accordingly. Please refer back to the supplied documentation.

A few converted examples from this PR:

feat(php/ng): support the use of a list of php versions
refactor(php/ng): don't iterate on string, make sure list is not string
style(pillar.example): add line break

Note, if you rebase this PR against the latest upstream, you'll get the Travis checks running in this PR. That will also run commitlint, to ensure that the commit messages are appropriately formatted.

arthurzenika · 2019-07-10T13:29:42Z

@myii great news, I'm eager to work with this tool. Does that mean I should do some git commit --amend then git push --force on my fork so that the existing commits get replaced ?

myii · 2019-07-10T14:11:34Z

@myii great news, I'm eager to work with this tool. Does that mean I should do some git commit --amend then git push --force on my fork so that the existing commits get replaced ?

@arthurlogilab Effectively, yes. When there are a number of commits needing their messages modified, I prefer to use git rebase <starting_point> -i. Then git push -f as you've mentioned.

Related to saltstack-formulas#138

arthurzenika · 2019-07-10T14:56:50Z

@myii done, lets see what travis says.

myii · 2019-07-10T15:31:48Z

You cracked it the first time, great job @arthurlogilab!

myii · 2019-08-02T04:18:21Z

@n-rodriguez Doesn't this need to be merged before #183?
@arthurlogilab Are you waiting for a specific reviewer before this can be merged?

arthurzenika · 2019-08-07T07:41:51Z

@myii not waiting for a specific reviewer, no. Thanks !

myii · 2019-08-07T09:21:03Z

Merged, thanks @arthurlogilab and to all others for their comments and reviews.

saltstack-formulas-travis · 2019-08-07T09:47:02Z

🎉 This PR is included in version 0.39.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

aboe76 requested review from alxwr and javierbertoli March 26, 2019 19:20

arthurzenika commented Apr 5, 2019

View reviewed changes

php/ng/fpm/config.sls Outdated Show resolved Hide resolved

arthurzenika commented Apr 5, 2019

View reviewed changes

alxwr self-assigned this Apr 9, 2019

philpep self-requested a review July 10, 2019 10:06

arthurzenika force-pushed the multiphp branch from dc3f505 to 6da7943 Compare July 10, 2019 14:49

arthurzenika and others added 6 commits July 10, 2019 16:52

feat(php/ng): support the use of a list of php versions

b303239

Related to saltstack-formulas#138

docs(pillar.example): example of versions

a98aa7e

fix(php/ng): don't iterate on string, make sure list is not string

dbb542c

feat(php/ng): support for php cli multiversion

bb4a077

docs(pillar.example): add example for alternatives with multiversion

23a6ec1

style(pillar.example): add line break

38fe58f

arthurzenika force-pushed the multiphp branch from 6da7943 to 38fe58f Compare July 10, 2019 14:53

myii merged commit ef25eb9 into saltstack-formulas:master Aug 7, 2019

myii mentioned this pull request Aug 7, 2019

feat(ng): promote NG formula #183

Merged

saltstack-formulas-travis added the released label Aug 7, 2019

Yoda-BZH mentioned this pull request Apr 16, 2020

Add example to use a specific php version when using multiple ones #212

Merged

19 tasks

Conversation

arthurzenika commented Mar 26, 2019

Uh oh!

arthurzenika commented Apr 5, 2019

Uh oh!

arthurzenika Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

arthurzenika Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arthurzenika Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

sschne commented Apr 12, 2019

Uh oh!

arthurzenika commented Apr 17, 2019

Uh oh!

sschne commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n-rodriguez commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurzenika commented Jul 10, 2019

Uh oh!

myii commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurzenika commented Jul 10, 2019

Uh oh!

myii commented Jul 10, 2019

Uh oh!

arthurzenika commented Jul 10, 2019

Uh oh!

myii commented Jul 10, 2019

Uh oh!

myii commented Aug 2, 2019

Uh oh!

arthurzenika commented Aug 7, 2019

Uh oh!

myii commented Aug 7, 2019

Uh oh!

saltstack-formulas-travis commented Aug 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sschne commented Apr 30, 2019 •

edited

Loading

n-rodriguez commented Jul 1, 2019 •

edited

Loading

myii commented Jul 10, 2019 •

edited

Loading