search

saltstack-formulas / php-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
57 stars 232 forks source link

A start with splitting map.jinja and php works in focal (Ubuntu 20.04) #214

Open waynegemmell opened 4 years ago

waynegemmell commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Does this PR introduce a BREAKING CHANGE?

No.

Describe the changes you're proposing

I wanted to install Focal packages and allow for sensible defaults for PHP versions. PHP FPM was also not honouring the php version when creating it's files. I started with separating the map.jinja file but I don't really know how to finish that job. I got far enough that the libraries now install with the correct version for the relevant Ubuntu distribution by default.

With this change now most packages install without specifying a version. php.cli is an exception. It would work if the php object was merged instead of being overwritten. I couldn't work that out.

Pillar / config required to test the proposed changes

Nothing

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

Best reviewed: commit by commit

Optimal code review plan

     Added osfamilymap, osmap and other defaults      Added osfamilymap, osmap and other defaults      Installs packages in focal.

Powered by Pull Assistant. Last update 817c54b ... 11fb54e. Read the comment docs.

danny-smit commented 3 years ago

This is actually a great improvement and could resolve a bugreport that I wanted to write. Is this almost done? What needs to be done to get this merged?

waynegemmell commented 3 years ago

I use it already. I'm sure some improvements could be made though, but it works for me atm.

On Wed, Oct 14, 2020 at 6:23 PM Danny Smit notifications@github.com wrote:

@danny-smit commented on this pull request.

In php/osfingermap.yaml https://github.com/saltstack-formulas/php-formula/pull/214#discussion_r504810152 :

@@ -0,0 +1,48 @@

+# -- coding: utf-8 --

+# vim: ft=yaml

+#

+# Setup variables using grains['osfinger'] based logic.

+# You just need to add the key:values for an osfinger that differ

+# from defaults.yaml + osarch.yaml + os_family.yaml + osmap.yaml.

+# Only add an osfinger which is/will be supported by the formula.

+#

+# If you do not need to provide defaults via the os_finger grain,

+# you will need to provide at least an empty dict in this file, e.g.

+# osfingermap: {}

+---

+# os: Debian

+Debian-10: {}

⬇️ Suggested change

-Debian-10: {}

+Debian-10:

  • pillar_php_version: "7.3"

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/php-formula/pull/214#pullrequestreview-508520580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFORIWZ5XAWFU77KCXMND3SKXF6VANCNFSM4NYX7DUA .

-- Regards Wayne Gemmell

myii commented 3 years ago

This is actually a great improvement and could resolve a bugreport that I wanted to write. Is this almost done? What needs to be done to get this merged?

@danny-smit Actually, the first part was waiting for @sticky-note to give some feedback. Otherwise, I would strongly suggest adding the new map.jinja verifier that we've started rolling across formulas (e.g. in the template-formula). That will help ensure that no regressions are introduced when making changes to the map. I might be able to provide something over the coming days.

Thanks for your patience, @waynegemmell.

waynegemmell commented 3 years ago

Glad to contribute :)

On Wed, Oct 14, 2020 at 9:13 PM Imran Iqbal notifications@github.com wrote:

This is actually a great improvement and could resolve a bugreport that I wanted to write. Is this almost done? What needs to be done to get this merged?

@danny-smit https://github.com/danny-smit Actually, the first part was waiting for @sticky-note https://github.com/sticky-note to give some feedback. Otherwise, I would strongly suggest adding the new map.jinja verifier that we've started rolling across formulas (e.g. in the template-formula). That will help ensure that no regressions are introduced when making changes to the map. I might be able to provide something over the coming days.

Thanks for your patience, @waynegemmell https://github.com/waynegemmell.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/php-formula/pull/214#issuecomment-708605569, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFORIVSLWFZC6FICCVCU5DSKXZ5FANCNFSM4NYX7DUA .

-- Regards Wayne Gemmell

danny-smit commented 3 years ago

@danny-smit Actually, the first part was waiting for @sticky-note to give some feedback. Otherwise, I would strongly suggest adding the new map.jinja verifier that we've started rolling across formulas (e.g. in the template-formula). That will help ensure that no regressions are introduced when making changes to the map. I might be able to provide something over the coming days.

I'm not familiar with that new verifier yet, but is there any way we can help with that?

myii commented 3 years ago

I'm not familiar with that new verifier yet, but is there anyway we can help with that?

@danny-smit We're just finalising the verifier to work on Windows as well:

Once that's merged, we'll pretty much have a standard verifier that can be used for most/all formulas. Hopefully sometime this week.

myii commented 3 years ago

@waynegemmell @danny-smit It's been a while but then the Travis CI curveball really threw us off. But we're getting on top of things now:

You'll get all of that if you rebase this PR. Will also need to consider how to deal with #215 at the same time (CC: @sticky-note).

danny-smit commented 3 years ago

Is that up to me? Can I do that? Since it is not my pull request.

waynegemmell commented 1 year ago

@myii @sticky-note I've done a rebase as recommended but it still seems to be failing. Any idea what's causing this? The logs aren't showing anything that I understand.