puppet-nginx icon indicating copy to clipboard operation
puppet-nginx copied to clipboard

Add support for log_format escape parameter

Open smortex opened this issue 2 years ago • 4 comments

Pull Request (PR) description

The escape parameter was added to nginx 1.11.8. This optional parameter allow 3 values: 'default', 'json' and 'none'. Setting this parameter is currently tricky do to the way the configuration snippet is generated (note the unbalanced quotes in the middle of the log string):

class { 'nginx':
  # ...
  log_format => {
    json_combined => "escape=json' '{...}",
    #                            ^ ^
    #                            | `-- start quote of the log string
    #                            `---- end quote of the escape format
  }
}

Adjust the data type of the log_format parameter to match the "legacy" way of only passing a Hash consisting of a name (String) matching a format string (String), but also accept a Tuple for the escape parameter (Enum) followed by the format string, allowing a less cluttered Puppet manifest:

class { 'nginx':
  # ...
  log_format => {
    json_combined => [
      'escape=json',
      '{...}',
    ],
  },
}

This Pull Request (PR) fixes the following issues

n/a

smortex avatar Sep 30 '21 00:09 smortex

nginx is a class

that may have no external impact to Forge modules.

This module is declared in 9 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Maybe it would be better to use a Variant[String,Struct[...]] to accommodate possible future parameters?

class { 'nginx':
  # ...
  log_format => {
    json_combined => {
      escape => 'json',
      format => '{...}',
    },
  },
}

Opinions from people using this module?

smortex avatar Oct 03 '21 00:10 smortex

This seems a little complex, suggest adding an example to the README.

ghoneycutt avatar Oct 03 '21 15:10 ghoneycutt

This seems a little complex, suggest adding an example to the README.

The data type is also lacking documentation. Maybe we can add examples at this level, I'll try in the next few days when I continue working on this at $WORK.

In the meantime, to you have an opinion regarding a Tuple (['escape=json', '{...}']) vs. a Struct ({ escape => 'json', format => '{...}' }) for the new way or providing a log format? I was first thinking about a Tuple because it allows a (little) less cumbersome template, but the extra validation when using a Struct would be good to have. Beyond this log_format example, I was thinking about access_log which is currently implemented using multiple parameters and where passing multiple arguments could be error-prone. I suspect there are many similar patterns in the module.

smortex avatar Oct 04 '21 05:10 smortex

Superseded by #1513. I completely forgot I already opened a PR for it.

smortex avatar Aug 24 '22 01:08 smortex