-
-
Notifications
You must be signed in to change notification settings - Fork 878
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
Allow specifying error_log severity for servers #1594
Conversation
You need to regenerate REFERENCE.md, see https://voxpupuli.org/docs/how_to_run_tests/ |
Can sb plz hint me why the tests may fail? I must have missed sth. |
The tests for this module are broken, I will merge a fix soon. After I did you need to re-base |
@Enrice if you re-base now the tests should look much better |
I was referring to the unit tests. Still no clue why they fail. |
You did not set the new option in the tests. I guess it would go here: https://github.com/voxpupuli/puppet-nginx/pull/1594/files#diff-26cc1a5b135079611a0a648adb7f94475b3eae5ca87348edb6933631ce8e79aeL72 But actually A dedicated test for the new parameter would be better |
That's what I did here e.g.: Still the defaults from the define (and the class in turn) don't seem to apply. |
Th part in line 361-372 look good. Simply revert the changes in lines 90, 350, 357-358. These are different tests... |
I don't understand why this should work, because I changed the default here: https://github.com/voxpupuli/puppet-nginx/pull/1594/files#diff-8967bbea230d7e3438686906bca67bccaa8b7f25820b28faf3a1d29494812493R403 So this change should also be reflected in the tests. However we can discuss if this breaking change makes sense. To me, it does. But maybe this should be split into separate features? |
I think this is fine to merge as an enhancement because it doesn't change the current log level? I also had to rebase it to cleanup the git history. |
improved fix for #1416