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

add support to access_log, error_log log_not_found per location

Open ceonizm opened this issue 2 years ago • 4 comments

Hello, This PR adds the support of the following directives at location level

  • access_log
  • error_log
  • log_not_found

it allows to be more precise in the logging strategy.

Hope it helps.

ceonizm avatar Oct 01 '21 05:10 ceonizm

nginx::resource::location is a type

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.

Hello Maintainers :)

By looking at theses test results it seems theses kind of errors are certainly not due to my code changes

Notice: /Stage[main]/Apt::Update/Exec[apt_update]/returns: E: Failed to fetch https://oss-binaries.phusionpassenger.com/apt/passenger/dists/focal/main/binary-amd64/Packages.gz File has unexpected size (8692 != 7637). Mirror sync in progress? [IP: 109.107.35.58 443]

and all others failures are dependent of theses not downloaded packages. So I guess with all the LetsEncrypt certificates's mess yesterday perhaps it was not the good time to post PR :)

is it possible to re-run the test suite when network issues will be fixed ?

Have a nice day

ceonizm avatar Oct 01 '21 07:10 ceonizm

Adding an acceptance test would make more casual users of this module feel better about pressing the merge button :)

ghoneycutt avatar Oct 03 '21 15:10 ghoneycutt

Adding an acceptance test would make more casual users of this module feel better about pressing the merge button :)

Hello @ghoneycutt Does the tests I've already added aren't sufficent ? https://github.com/ceonizm/puppet-nginx/commit/cbd614edd87d5a10a75251cc635919f8460038af#diff-254ab018dcc706b1c8f6aa90e170616c3a13d5fad27a90ed1c830ea4efe83b37

I however agree that there may still a case not tested when access_log is used jointly with log_format because I didn't knew how to write a such test with two params. (for now as you can see I've just mimiced the way existing tests were done by adding my case to the existing array).

I'd love to know what to add :)

ceonizm avatar Oct 03 '21 18:10 ceonizm