feat(ng): promote NG formula

[sticky-note]

This PR include:

• Promotion of NG formula
• Introduction of recommended tplroot var
• {%- consistency

PS: I've not yet tested these changes. I began this work to have feedback first. Ping @myii @n-rodriguez @aboe76

[myii]

@sticky-note In terms of the deprecation messages in cf05b73, if you look at https://github.com/saltstack-formulas/nginx-formula/pull/236/files, you'll see that we have to add these to the states that we're deprecating, i.e. the non-ng states that are going to be removed.

[myii]

Copying across our earlier conversation from Slack:

-	-	-
00:28	zabiteus93	Hi folks, can someone help me with this ? https://github.com/saltstack-formulas/php-formula/pull/183
00:33	myii	@​zabiteus93 Apologies, very busy time at the moment. I saw the PR and was meaning to give you some initial feedback there.
00:34	myii	Now, with ng conversions, we've got to do this over stages. We went a little too fast with the nginx-formula, with no time to give warnings beforehand.
00:35	myii	Since then, we put the warnings in retrospectively but that really should have been the first stage.
00:35	myii	So taking your PR in general, that's a bit further down the line -- we have to build towards it.
00:36	myii	What we need first is a deprecation warning in the next "release" of php-formula. It can use a similar method to the one used by nginx-formula.
00:38	myii	This PR is the basis for that, which links to the others: https://github.com/saltstack-formulas/nginx-formula/pull/236.
00:40	myii	This time around, since we're only warning at first, change both test.fail_without_changes and failhard: True -- these will be reintroduced once the deprecation has taken place.
00:41	myii	And the message should be modified likewise.
00:41	myii	All of this requires another PR first. Are you ready to do that @​zabiteus93?
02:38	zabiteus93	I have to dig into some regression first and yes, I'm really enthusiast to do that
02:39	zabiteus93	If somenone can help me with some test, it'll be really aspreciated
05:11	zabiteus93	deprecated state done : //github.com/saltstack-formulas/nginx-formula/pull/236Debian 9 travis check fails, don't know why yet

[myii]

@sticky-note As I mentioned in Slack, it is better if we merge the deprecations warnings first, in a separate PR. We want to release a version that starts warning people to get ready to move across to php:ng. That way, we don't have too much going on in this PR as well. It is already too much to review, with over 200+ files. It would be great if we could somehow do this in smaller stages.

Any suggestions @n-rodriguez, @aboe76, @daks?

[johnkeates]

Same as with nginx:ng (but that went wrong IIRC)

[daks]

In fact, nginx NG promotion has been done quite quickly and we have learnt from it, so I'm also in favour of doing a release to add the deprecation warnings.

[n-rodriguez]

I'm also in favour of doing a release to add the deprecation warnings.

:+1:

[myii]

First merge a PR with deprecation warnings with a major release, then Promote ng with another major release ?

@sticky-note That's right but the deprecation warnings PR doesn't have to be a major release. A couple of points about what needs to be done:

1. Each state file under php (non-ng) needs a deprecation warning included, explaining that these files will be removed in upcoming v1.0.0.
2. Each state file under php.ng needs an explanatory message included, informing that these files will be promoted to php in v1.0.0 (i.e. that the .ng will be dropped).

Obviously, all of these using an include at the top of the file.

[sticky-note]

@myii I have some time to edit this PR I had to merge master into this branch because of multiple rebases How can I get rid of the merge commit ? We need to modify the warning messages too Do we need also to leave the ng folder with all of the older .sls with an

include:
  - php.ng.error

or something ?

[myii]

Just merged #167, so a little more rebasing here @sticky-note, sorry!

[myii]

@sticky-note Hold on, I'll try to get you something patched up.

[myii]

@sticky-note OK, I've patched things up and put it together in one commit on top of the latest upstream here: https://github.com/myii/php-formula/commit/7fbb40b007464c62581dbc4d2a933016be3406a3. Have a look and see how that compares with the work you have done.

The problem here is that too many changes are being made at the same time, which makes it difficult for git to follow what's going on. For example, when patching this again, it kept tripping up on:

• All of the {% which have been fixed to {%-
• The introduction of tplroot via. {%- set tplroot = tpldir.split('/')[0] %}

While these are useful changes (to make eventually), these should be left for a subsequent PR, so that this one can be applied cleanly first. In fact, it may still be the sensible option to do that, so that we can be completely confident that these changes are sound. We can always compare with the linked branch, to reintroduce the extra changes in a subsequent PR.

[sticky-note]

https://github.com/saltstack-formulas/php-formula/pull/183#issuecomment-519017038 No problem

• I've removed php.ng.deprecated.sls andphp.deprecated.sls from pre-commit_semantic-release.sh and release.config as well
• Copied the Readme.rst message from nginx fomula

Seems to be correct but Needs real tests

Do we need to leave ng state with an error.sls include or something ?

[myii]

@sticky-note This is what we did in the the nginx-formula.

[sticky-note]

@myii And voilà ?

[myii]

@sticky-note Fantastic effort! I haven't looked in specific detail (the very latest changes) but this really looks like it's made a lot of progress. We really need some testing on this as well. I don't use this formula for real, so let's try to ping some of the main contributors to see if they can review/test this PR before we perform this deprecation.

CC: @johnkeates @daks @n-rodriguez @alxwr @aboe76 @arthurlogilab.

[myii]

@n-rodriguez The debian test fails intermittently because the php package sometimes isn't installed. Should the test be updated to check for the php7.0 package instead?

[aboe76]

will try to to test this, on the weekends, hopefully I can find some time.

[aboe76]

@sticky-note first test missing Arch support in map.jinja.

[aboe76]

@sticky-note on Debian I got a nice deprecated warning message and I see no issues

[myii]

@aboe76 @sticky-note Just be aware that this PR will need to include the fix in #189.

[myii]

Or probably better to rebase on it once it is merged.

[myii]

@sticky-note I've merged #189 and a new release has been created. I've performed the rebase of this PR locally, including fixing the commit message to ensure that a new release is created. Do you want me to push it here instead of this commit (obviously still with your authoring details)?

[sticky-note]

@myii I've managed to rebase from actual master I've fixed the release number on README.rst I don't know debian 9 test is not passing any more

[myii]

@sticky-note As for the Debian failure, that's what I mentioned above: https://github.com/saltstack-formulas/php-formula/pull/183#issuecomment-519291286. See:

• https://travis-ci.com/saltstack-formulas/php-formula/jobs/224627690#L712
• https://travis-ci.com/saltstack-formulas/php-formula/jobs/224627690#L768

We can solve this in a tiny separate PR before/after this PR.

[sticky-note]

I confirm php7.0 only is available from official Debian repos

[sticky-note]

Made the two requested changes And now tests pass in Ubuntu and Debian with that PR https://github.com/saltstack-formulas/php-formula/pull/190 Can I cherry-pick from there or I wait to be able to rebase master ? Thanks @myii

[myii]

@sticky-note Thanks for the changes. Let's wait a little bit, so that #190 can be merged and that any other contributors can check this PR.

@aboe76 How many reviews do you think we need before merging this?

[aboe76]

If we have a confirmation that it works on the major distro's I'm fine:

• Debian/Ubuntu
• Redhat/Centos

The others can be fixed with later PR's,

[sticky-note]

I've just rebased from master and fixed release number on README.rst and php/deprecated.sls. Can I remove the WIP prefix on the PR name ?

[myii]

@sticky-note Yes, please do that. This is almost ready for merging now, thanks.

[myii]

If we have a confirmation that it works on the major distro's I'm fine:

• Debian/Ubuntu
• Redhat/Centos

@aboe76 You mentioned that you tested Debian above, so just the other three remain?

[aboe76]

@myii you are correct but having others testing it with different parameters would provide a better result.

[sticky-note]

Okay for minor edits. Works for me on debian 9 and FreeBSD 12

[aboe76]

Can somebody confirm this works on Centos/RedHat or Fedora?

[myii]

@philpep (and @arthurlogilab) may be able to help us with CentOS: https://github.com/saltstack-formulas/php-formula/pull/191#issuecomment-521274025.

[myii]

@sticky-note I'm sorry, I was holding back on #191 so that this could be merged first, in order to avoid another rebase, but I didn't put a warning on my own PR #192! Since that was merged and this needs to be rebased anyway, I've merged #191 as well. I'm happy to perform the rebase on this, since it's my fault in a way. Are you OK with that?

[myii]

@sticky-note So I've performed the rebase on my fork, including incorporating all of the PRs that have been merged since this PR was started:

• https://github.com/myii/php-formula/commit/5aade39da876cab70676f2b7dbfd89e56e650599

I hope this is the last time we have to do this because it takes a lot of effort.

Either I can push the commit here directly, or you can at least compare to that commit when performing the rebase yourself. As you can see, the commit is still authored with your details. It's up to you how you want to proceed.

[sticky-note]

@myii, I've rebased with master Not sure if pillar.example is correct

[myii]

I've rebased with master

@sticky-note Sorry again for the trouble. Let's hope this is the last time.

Not sure if pillar.example is correct

It appears to be fine. What do you think is incorrect?

[sticky-note]

@myii

It appears to be fine. What do you think is incorrect?

I've not digged in details to the very last modifications. I've just trusted the mergetool with some fixes concerning ng If you think it is correct, so go to finalize this PR. What to to next ?

[myii]

What to to next ?

@sticky-note That's all from my end and hopefully from your end. We're just waiting for the final confirmations before this can be merged. Looking above, it's what @aboe76 has mentioned:

Can somebody confirm this works on Centos/RedHat or Fedora?

@philpep @arthurlogilab Can either of you do that?

[sticky-note]

Want to test it on CentOS with vagrant to save some time. Is it ok @myii ?

[myii]

Want to test it on CentOS with vagrant to save some time. Is it ok @myii ?

Sure, it just needs to be confirmed and then we should be able to proceed. Is that OK, @aboe76?

[sticky-note]

@myii @aboe76 States for php-fpm, fpm-pools and around twenty php modules, and composer installation with scl repo enabled and php_version : 72 are confirmed to work on a CentOS 7 vagrant box

[myii]

Thanks @sticky-note, I'm happy for this to be merged. Any final comments @aboe76, @n-rodriguez @daks @johnkeates?

[aboe76]

Lgtm

[n-rodriguez]

Lgtm

same here :+1:

[myii]

Congratulations @sticky-note, it's merged -- thanks for your hard work and patience!

Appreciate all of the feedback and reviews from everyone else.

[saltstack-formulas-travis]

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

[myii]

@sticky-note The commit message fixes must have got lost during all of the rebases, so it didn't trigger a v1.0.0. See https://github.com/myii/php-formula/commit/5aade39da876cab70676f2b7dbfd89e56e650599:

BREAKING CHANGE: all previous `php` based configurations must be reviewed;
`php.ng` usage must be promoted to `php` and any uses of the original
`php` will have to be converted.

Since BREAKING CHANGES (extra S) was used instead, that's why this happened. Don't worry, I'll add a commit now to fix this.