Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for log_format escape parameter #1470

Closed
wants to merge 1 commit into from
Closed

Conversation

smortex
Copy link
Member

@smortex smortex commented Sep 30, 2021

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

@puppet-community-rangefinder
Copy link

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.

@smortex smortex added the enhancement New feature or request label Sep 30, 2021
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):

```puppet
class { 'nginx':
  # ...
  log_format => {
    json_combined => "escape=json' '{...}",
    #                            ^ ^
  }
}
```

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:

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

[escape parameter]: https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format
@smortex
Copy link
Member Author

smortex commented Oct 3, 2021

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?

@ghoneycutt
Copy link
Member

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

@smortex
Copy link
Member Author

smortex commented Oct 4, 2021

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 smortex marked this pull request as draft October 4, 2021 05:55
@smortex
Copy link
Member Author

smortex commented Aug 24, 2022

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

@smortex smortex closed this Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants