[BUG] typo make_dirs vs makedirs in php-formula/php/fpm/config.sls

[gj02ib65]

Describe the bug

While using php-formula I found that directories were not being created and discovered a typo of make_dirs rather than makedirs. I have changed and that now works.

Setup

Steps to reproduce the bug

Expected behaviour

Versions report

As mentioned in #200.

Additional context

Found that I needed to re-arrange the code a bit and create the directory before using managed files. This is also in the same file, there are 2 places where I moved the file.directory block in front of the file.managed section. Following is the updated config.sls:

{#- Manages the php-fpm main ini file #}
{%- set tplroot = tpldir.split('/')[0] %}
{%- from tplroot ~ "/map.jinja" import php with context %}
{%- from tplroot ~ "/ini.jinja" import php_ini %}

{%- set ini_settings = php.ini.defaults %}
{%- for key, value in php.fpm.config.ini.settings.items() %}
  {%- if ini_settings[key] is defined %}
    {%- do ini_settings[key].update(value) %}
  {%- else %}
    {%- do ini_settings.update({key: value}) %}
  {%- endif %}
{%- endfor %}

{%- set pillar_php_version = salt['pillar.get']('php:version', '7.0') %}
{%- if pillar_php_version is iterable and pillar_php_version is not string %}
  {%- for version in pillar_php_version %}
    {%- set conf_settings = odict(php.lookup.fpm.defaults) %}
    {%- set first_version = pillar_php_version[0]|string %}
    {%- set ini = php.lookup.fpm.ini|replace(first_version, version) %}
    {%- set conf = php.lookup.fpm.conf|replace(first_version, version) %}
    {%- set pools = php.lookup.fpm.pools|replace(first_version, version) %}

    {%- for key, value in conf_settings.items() %}
      {%- if value is string %}
        {%- do conf_settings.update({key: value.replace(first_version, version)}) %}
      {%- 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' }) %}

{{ pools }}:
    file.directory:
        - name: {{ pools }}
        - user: {{ php.lookup.fpm.user }}
        - group: {{ php.lookup.fpm.group }}
        - file_mode: 755
        - makedirs: True
        
php_fpm_ini_config_{{ version }}:
  {{ php_ini(ini,
             'php_fpm_ini_config_' ~ version,
             php.fpm.config.ini.opts,
             ini_settings
  ) }}

php_fpm_conf_config_{{ version }}:
  {{ php_ini(conf,
             'php_fpm_conf_config_' ~ version,
             php.fpm.config.conf.opts,
             odict(conf_settings)
  ) }}
  {%- endfor %}
{%- else %}

{%- set conf_settings = php.lookup.fpm.defaults %}
{%- do conf_settings.update(php.fpm.config.conf.settings) %}

{{ php.lookup.fpm.pools }}:
    file.directory:
        - name: {{ php.lookup.fpm.pools }}
        - user: {{ php.lookup.fpm.user }}
        - group: {{ php.lookup.fpm.group }}
        - file_mode: 755
        - makedirs: True

php_fpm_ini_config:
  {{ php_ini(php.lookup.fpm.ini,
             'php_fpm_ini_config',
             php.fpm.config.ini.opts,
             ini_settings
  ) }}

php_fpm_conf_config:
  {{ php_ini(php.lookup.fpm.conf,
             'php_fpm_conf_config',
             php.fpm.config.conf.opts,
             conf_settings
  ) }}
{%- endif %}

---

Optional: How can this template be improved?

[myii]

@gj02ib65 Since the issue isn't resolved in this repo, I'll re-open the issue so that it can finally be resolved.

[ghormoon]

is there a reason not to pull the same commit?
also the error_log directory is not created at all (not even any relevant code exists), which makes it fail starting the service on some distros that don't create it from package automatically

[myii]

is there a reason not to pull the same commit?
also the error_log directory is not created at all (not even any relevant code exists), which makes it fail starting the service on some distros that don't create it from package automatically

@ghormoon No reason but someone needs to look at that commit to check the reordering of states as well. The make_dirs => makedirs typo should be done in all cases, of course.

[ghormoon]

I think the thought behind the reordeing is that to have the directory already ready when creating php-fpm.conf (as it likely is in a directory above), as it might fail too, if the folder is missing (sort of similar case as the log one, but less likely as those are usually in packages), so kinda hacky solution for that (it would be cleaner to do it explicitly, would make one more state, but in case someone has the pools outside the php/fpm folder where php-fpm.conf is, it will handle that edgecase)

[myii]

I think the thought behind the reordeing is that to have the directory already ready when creating php-fpm.conf (as it likely is in a directory above), as it might fail too, if the folder is missing (sort of similar case as the log one, but less likely as those are usually in packages), so kinda hacky solution for that (it would be cleaner to do it explicitly, would make one more state, but in case someone has the pools outside the php/fpm folder where php-fpm.conf is, it will handle that edgecase)

@ghormoon Yes, perhaps using requisites is the right way, whether reordering or not. The solution shouldn't be a hack, ultimately.