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

Allow specifying error_log severity for servers #1594

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

Enrice
Copy link

@Enrice Enrice commented May 22, 2024

improved fix for #1416

@Enrice Enrice marked this pull request as ready for review May 22, 2024 08:57
@TheMeier
Copy link
Contributor

TheMeier commented Jun 2, 2024

You need to regenerate REFERENCE.md, see https://voxpupuli.org/docs/how_to_run_tests/

@Enrice
Copy link
Author

Enrice commented Jun 4, 2024

Can sb plz hint me why the tests may fail? I must have missed sth.

@TheMeier
Copy link
Contributor

TheMeier commented Jun 5, 2024

The tests for this module are broken, I will merge a fix soon. After I did you need to re-base

@TheMeier TheMeier added the enhancement New feature or request label Jun 5, 2024
@TheMeier TheMeier added this to the 6.0.0 milestone Jun 5, 2024
@TheMeier
Copy link
Contributor

TheMeier commented Jun 5, 2024

@Enrice if you re-base now the tests should look much better

REFERENCE.md Show resolved Hide resolved
@Enrice
Copy link
Author

Enrice commented Jun 5, 2024

I was referring to the unit tests. Still no clue why they fail.

@TheMeier
Copy link
Contributor

TheMeier commented Jun 5, 2024

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

@Enrice
Copy link
Author

Enrice commented Jun 5, 2024

That's what I did here e.g.:
https://github.com/voxpupuli/puppet-nginx/pull/1594/files#diff-26cc1a5b135079611a0a648adb7f94475b3eae5ca87348edb6933631ce8e79aeR361

Still the defaults from the define (and the class in turn) don't seem to apply.

@TheMeier
Copy link
Contributor

TheMeier commented Jun 5, 2024

Th part in line 361-372 look good. Simply revert the changes in lines 90, 350, 357-358. These are different tests...

@Enrice
Copy link
Author

Enrice commented Jun 5, 2024

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.
Reason: we EXPLICITLY default the severity on class level to 'error' which would be the nginx default anyway. So it is legit to have it EXPLICITLY configured on the server resource as well, so I followed this pattern.

But maybe this should be split into separate features?

@bastelfreak bastelfreak changed the title Allow specifying error_log severity for servers, improved Allow specifying error_log severity for servers Jun 7, 2024
@bastelfreak
Copy link
Member

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.

@TheMeier TheMeier merged commit 77b2188 into voxpupuli:master Jun 7, 2024
27 checks passed
@Enrice Enrice deleted the fix-1416 branch June 7, 2024 11:45
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.

None yet

3 participants