Fix `apache:lookup` regression

[myii]

Regression

Regression mentioned on the # irc channel yesterday (2018-01-27):

XenophonF APP [17:51] this commit completely broke apache-formula https://github.com/saltstack-formulas/apache-formula/commit/094b149262ab7355be37832c337f4e76150e525d

XenophonF APP [17:52] now the apache:lookup Pillar key gets overridden by the various maps instead of the other way around (as documented)

• Probably @xenophonf here on GitHub?

My response to that:

I'm intrigued by this issue and I have asked previously but haven't got a satisfactory answer. What is the merging priority in map.jinja?

So in the apache-formula currently:

• defaults < apache:lookup < modsecurity < osfamily < oscode < osfinger < apache

And I understand that you are suggesting:

• defaults < modsecurity < osfamily < oscode < osfinger < apache:lookup < apache

This makes sense. I'd be interested in chasing this up, since we've just committed a similar change to the salt-formula @ https://github.com/saltstack-formulas/salt-formula/pull/395, which may also need to be revised.

You mentioned (as documented) -- can you link me to this so that it can be brought up for discussion?

Regression confirmation

Using a stripped down version of pillar.example:

apache:
  # lookup section overrides ``map.jinja`` values
  lookup:
    server: apache2
    service: apache2
    user: some_system_user
    group: some_system_group

    vhostdir: /etc/apache2/sites-available
    confdir: /etc/apache2/conf.d
    confext: .conf
    logdir: /var/log/apache2
    wwwdir: /srv/apache2

    # apache version (generally '2.2' or '2.4')
    version: '2.2'

    # ``apache.mod_wsgi`` formula additional configuration:
    mod_wsgi: mod_wsgi

    # Default value for AddDefaultCharset in RedHat configuration
    default_charset: 'UTF-8'

    # Should we enforce DocumentRoot user/group?
    # Default: do not enforce
    document_root_user: www-data   # Force user if specified, leave it default if not
    document_root_group: null      # Do not enforce group

Here's the diff of the generated map.jinja before and after this commit (excluding the extra lookup key):

--- e2462b2
+++ c4154ba
@@ -1,5 +1,5 @@
 {
-  "confdir": "/etc/apache2/conf.d",
+  "confdir": "/etc/apache2/conf-available",
   "confext": ".conf",
   "configfile": "/etc/apache2/apache2.conf",
   "default_charset": "UTF-8",
@@ -7,9 +7,10 @@
   "default_site_ssl": "default-ssl.conf",
   "document_root_group": null,
   "document_root_user": "www-data",
-  "group": "some_system_group",
+  "group": "www-data",
   "logdir": "/var/log/apache2",
   "logrotatedir": "/etc/logrotate.d/apache2",
+  "manage_service_states": true,
   "mod_fastcgi": "libapache2-mod-fastcgi",
   "mod_fcgid": "libapache2-mod-fcgid",
   "mod_geoip": "libapache2-mod-geoip",
@@ -24,13 +25,13 @@
     "manage_config": false,
     "package": "libapache2-mod-security2"
   },
-  "mod_wsgi": "mod_wsgi",
+  "mod_wsgi": "libapache2-mod-wsgi",
   "mod_xsendfile": "libapache2-mod-xsendfile",
   "portsfile": "/etc/apache2/ports.conf",
   "server": "apache2",
   "service": "apache2",
-  "user": "some_system_user",
+  "user": "www-data",
   "version": "2.2",
   "vhostdir": "/etc/apache2/sites-available",
-  "wwwdir": "/srv/apache2"
+  "wwwdir": "/srv"
 }

Summary of changes

1. Primary changes was to move the merge of apache:lookup as the penultimate merge in the map.
2. Took this as an opportunity to use salt['defaults.merge'] in place of .update, to ensure merging (to avoid issues such as https://github.com/saltstack-formulas/salt-formula/pull/395#issuecomment-457820419).

The diff with the "before" commit shows that the regression is resolved (excluding the extra lookup key):

--- e2462b2
+++ d10254d
@@ -10,6 +10,7 @@
   "group": "some_system_group",
   "logdir": "/var/log/apache2",
   "logrotatedir": "/etc/logrotate.d/apache2",
+  "manage_service_states": true,
   "mod_fastcgi": "libapache2-mod-fastcgi",
   "mod_fcgid": "libapache2-mod-fcgid",
   "mod_geoip": "libapache2-mod-geoip",
@@ -29,6 +30,8 @@
   "portsfile": "/etc/apache2/ports.conf",
   "server": "apache2",
   "service": "apache2",
+  "service_enable": true,
+  "service_state": "running",
   "user": "some_system_user",
   "version": "2.2",
   "vhostdir": "/etc/apache2/sites-available",

---

Update: The centos-7 build failed the first time but passed when I bumped it.

[myii]

Justification for salt['defaults.merge'] over .update

Regarding point raised by @noelmcloughlin above:

- {% do defaults.apache.update(oscode) %}
+ {% do salt['defaults.merge'](defaults['apache'], oscode) %}

This is easiest to explain using the recent issue encountered in https://github.com/saltstack-formulas/salt-formula/pull/395#issuecomment-457820419.

defaults.yaml#L32-L49:

  gitfs:
    dulwich:
      install_from_source: True
    pygit2:
      install_from_source: True
      version: 0.23.0
      git:
        # if not false, should be state name
        require_state: False
        install_from_package: git
      libgit2:
        version: 0.23.0
        install_from_source: True
        build_parent_dir: /usr/src/
        # hash necessary until github issue #9272 is addressed
        download_hash: 683d1164e361e2a0a8d52652840e2340
    gitpython:
      install_from_source: False

osfamilymap.yaml#L18-L26 (for Debian):

  gitfs:
    pygit2:
      install_from_source: True
      version: 0.22.1
      git:
        require_state: False
        install_from_package: git
      libgit2:
        install_from_source: False

osmap.yaml#L20-L25 (for Ubuntu):

  gitfs:
    pygit2:
      install_from_source: False
      git:
        require_state: False
        install_from_package: Null

The difference between the two methods:

--- Using `.update` throughout
+++ Using `salt['defaults.merge']` throughout
@@ -1,9 +1,22 @@
   "gitfs": {
+    "dulwich": {
+      "install_from_source": true
+    },
+    "gitpython": {
+      "install_from_source": false
+    },
     "pygit2": {
       "git": {
         "install_from_package": null,
         "require_state": false
       },
-      "install_from_source": false
+      "install_from_source": false,
+      "libgit2": {
+        "build_parent_dir": "/usr/src/",
+        "download_hash": "683d1164e361e2a0a8d52652840e2340",
+        "install_from_source": false,
+        "version": "0.23.0"
+      },
+      "version": "0.22.1"
     }
   },

• The osfamilymap update overwrites the entire gitfs key, losing all of the dulwich and gitpython defaults, as well as losing most of the pygit2:libgit2 defaults.
• The osmap update overwrites the entire gitfs key again, with further losses of pygit2:version and the rest of pygit2:libgit2.
• Whereas the merge keeps everything from each previous level, only changing values where different.

Of course, there are use-cases for using .update, when overwriting an entire key is the required functionality.

[aboe76]

@myii nice fix thanks

[myii]

@aboe76 Thanks for the review and merge.