-
-
Notifications
You must be signed in to change notification settings - Fork 136
If the relay parameter is not set, the empty? errors out. #449
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, seeing how we're doing the same check in the next line anyway.
Description could've used a bit more love. :) |
Sorry, thought I had put in a better description. No idea where I pasted that 8-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I started to write a review but realized I had not finished it and it was still pending.
I feel like better / more complete unit test in this area would have helped spot this issue, but it is another story.
@@ -232,7 +232,7 @@ def create | |||
|
|||
case resource[:backend].to_s | |||
when 'relay' | |||
t << "olcRelay: #{resource[:relay]}\n" unless resource[:relay].empty? | |||
t << "olcRelay: #{resource[:relay]}\n" if resource[:relay] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty string/array now evaluates to a truthy value, so this will change the code behavior.
If we can also pass undef
it may make sense to check resource[:relay].nil?
and resource[:relay].empty?
:
t << "olcRelay: #{resource[:relay]}\n" if resource[:relay] | |
t << "olcRelay: #{resource[:relay]}\n" unless resource[:relay].nil? || resource[:relay].empty? |
You can describe the wrong behavior you fix in the PR first message. It will be linked to in the generated ChangeLog with the next release of the module. |
Pull Request (PR) description
This Pull Request (PR) fixes the following issues