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 to access_log, error_log log_not_found per location #1471

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ceonizm
Copy link
Contributor

@ceonizm ceonizm commented Oct 1, 2021

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.

@puppet-community-rangefinder
Copy link

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.

@ceonizm
Copy link
Contributor Author

ceonizm commented Oct 1, 2021

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

@smortex smortex added the enhancement New feature or request label Oct 1, 2021
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I restarted the CI and the transient errors are gone. The remaining errors on CentOS are handled in #1473 and you can ignore these!

Just a minor suggestion if it looks right to you.

Thanks!

manifests/resource/location.pp Outdated Show resolved Hide resolved
Co-authored-by: Romain Tartière <[email protected]>
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I would like to have another review before merging because I am not a "serious" user of this module 😃

@ghoneycutt
Copy link
Member

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

@ceonizm
Copy link
Contributor Author

ceonizm commented Oct 3, 2021

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 ?
ceonizm@cbd614e#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 :)

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.

3 participants