update resolver parameters #526
Conversation
haproxy::resolver is a typeBreaking changes to this file WILL impact these 1 modules (exact match):This module is declared in 40 of 579 indexed public
|
|
This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days. |
| $hold = undef, | ||
| $resolve_retries = undef, | ||
| $timeout = undef, | ||
| $accepted_payload_size = undef, | ||
| # https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#5.3.2-accepted_payload_size | ||
| Optional[Integer[512, 8192]] $accepted_payload_size = undef, |
There was a problem hiding this comment.
Could you align the different variable lengths
There was a problem hiding this comment.
Sure, done; I did this as an additional commit rather than mix functional and whitespace changes together.
|
@bugfood One small change is needed, otherwise this looks good Also, sorry on the wait for a review |
|
Hi @bugfood - this change looks good. Can you address @david22swan's comment above, and rebase with the current main, so we can proceed with this. |
|
Thanks, yes, I will take a look at this. |
This fixes the error: Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Comparison of: Undef Value < Integer, is not possible. Caused by 'Only Strings, Numbers, Timespans, Timestamps, and Versions are comparable'. (file: /etc/puppetlabs/code/modules/haproxy/manifests/resolver.pp, line: 125, column: 30) HAProxy allows this parameter to be unset, and the code here works with this ok, aside from the bounds check. Replace the bounds check with a parameter type declaration, so that puppet itself will do the check for us. Update tests accordingly.
|
@jordanbreen28 @david22swan thanks, I amended the PR for:
I've been using my branch on live hosts since I made the PR, and it's been fine so far, at least for my use case. The rebased branch works as well. |
|
@bugfood thank you for all your work on this so far! |
The parse-resolv-conf parameter can be used on its own or in conjunction with the nameservers parameter. Adjust the nameservers parameter default to allow the template to work; an empty hash results in no nameservers specified. Add a new test.
Yes, rubocop seems to be getting stricter over time. I fixed that failure, and I don't see any other failures in the test report. Hopefully the test suite will pass now. |
|
Note that there is a deprecation warning on This warns on the test I added, but that's only because the new test is listed first in the file. The new tests I added follow the same construction as the other tests, and the same warning happens on current upstream I don't think that the pre-existing warning needs to be fixed here, but if so, I would need some further guidance on that. The string "should" does not appear in that file; I presume that some higher-level construct is using ":should", but I'm not familiar enough with rspec to know what would need to be changed. Other examples of that error I can find by a web search are for code that looks rather different. |
|
Hi again @bugfood :-) |
|
Very good. Thank you for helping me with me PRs. |
This fixes two separate issues with the
resolverdefined type:accepted_payload_sizepuppet parameter must be specified, despite HAProxy itself allowing the corresponding option to be unspecified.parse-resolv-confHAProxy option is not supported.I have updated the tests accordingly.