feat(odbc): add odbc module support

javierbertoli commented 4 years ago

PR progress checklist (to be filled in by reviewers)

[ ] Changes to documentation are appropriate (or tick if not required)
[ ] Changes to tests are appropriate (or tick if not required)
[ ] Reviews completed

What type of PR is this?

Primary type

[ ] [build] Changes related to the build system
[ ] [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ ] [ci] Changes to the continuous integration configuration
[x] [feat] A new feature
[ ] [fix] A bug fix
[ ] [perf] A code change that improves performance
[ ] [refactor] A code change that neither fixes a bug nor adds a feature
[ ] [revert] A change used to revert a previous commit
[ ] [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[ ] [docs] Documentation changes
[ ] [test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Describe the changes you're proposing

This PR adds basic support to install php-odbc packages, following the current states.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

[ ] Updated the README (e.g. Available states).
[ ] Updated pillar.example.

Testing checklist

[ ] Included in Kitchen (i.e. under state_top).
[ ] Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
[ ] Updated the relevant test pillar.

Additional context

myii commented 4 years ago

Works for Debian and Ubuntu (as it has been configured to):

https://travis-ci.org/myii/php-formula/builds/622905938

A question (I don't use this formula): is it usual to have something like this defined for only some OSes? Furthermore, should we be testing for these additions as well?

Nothing to prevent this PR from being merged, of course.

javierbertoli commented 4 years ago

A question (I don't use this formula): is it usual to have something like this defined for only some OSes?

I understand Debian family splits the php modules in different packages, not quite sure what other distros do. I was just scratching a minor itch, and just did a quick addition following what I saw in the formula :yum:

Furthermore, should we be testing for these additions as well?

Perhaps? I saw some packages are being tested, I thought they were just 'a few to see things work'. Do you want me to add it to the tests?

Nothing to prevent this PR from being merged, of course.

I think this formula could benefit from some major refactoring, specially in the map.jinja and modules installation in the future?

myii commented 4 years ago

Perhaps? I saw some packages are being tested, I thought they were just 'a few to see things work'. Do you want me to add it to the tests?

I recall a conversation between a few of us (?) at some point, where we were discussing about levels of testing.

Ideally, writing tests in InSpec (or at least saltcheck).
At the very least, running most/all of the states via. state_top.
- Even that would help to catch issues, such as this one, which we faced yesterday
As for this formula, that's an awful lot of states, so I'm not sure how effective it will be.

I think this formula could benefit from some major refactoring, specially in the map.jinja and modules installation in the future?

Now that would be a massive contribution to this formula. The current map.jinja is exceptionally difficult to work with and debug.

aboe76 commented 4 years ago

@myii and @javierbertoli, the map.jinja grew massively with the changes in distro's to ship php5/6/7 versions and have them configured differently because the php project also moved some config.

Refactoring this, will be definitely a massive task. I tried it once and gave up.

My thoughts on this is split the formula into different formula's:

php-formula -> core php config
php-module-formula -> formula per php module
php-fpm-formula -> php fpm only.

This way the jinja logic will be smaller to handle. but it will increase the number of formula's drastically.

myii commented 4 years ago

Let's get this merged for now. Thanks @javierbertoli and @aboe76.

saltstack-formulas-travis commented 4 years ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

saltstack-formulas / php-formula