[php/ng] support the use of a list of php versions

saltstack-formulas / php-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html

Other

57 stars 232 forks source link

[php/ng] support the use of a list of php versions #167

Closed arthurzenika closed 5 years ago

arthurzenika commented 5 years ago

Related to #138

arthurzenika commented 5 years ago

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

sschne commented 5 years ago

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 commented 5 years ago

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

sschne commented 5 years ago

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 commented 5 years ago

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

arthurzenika commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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.

arthurzenika commented 5 years ago

@myii done, lets see what travis says.

myii commented 5 years ago

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

myii commented 5 years ago

@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 commented 5 years ago

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

myii commented 5 years ago

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

saltstack-formulas-travis commented 5 years ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket: