-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Use .blank? instead of .empty? when handling SSH Key details to prevent crashes #17416
Conversation
print_status(" Description: #{e[1]}") if !e[1].nil? || !e[1].blank? | ||
print_status(" Passphrase: #{e[2]}") if !e[2].nil? || !e[2].blank? | ||
print_status(" Username: #{e[3]}") if !e[3].nil? || !e[3].blank? |
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.
.blank?
would also check if the object is nil
as well and so the first part of this code would not be needed. Might I suggest this instead to simplify the code?
print_status(" Description: #{e[1]}") if !e[1].nil? || !e[1].blank? | |
print_status(" Passphrase: #{e[2]}") if !e[2].nil? || !e[2].blank? | |
print_status(" Username: #{e[3]}") if !e[3].nil? || !e[3].blank? | |
print_status(" Description: #{e[1]}") if !e[1].blank? | |
print_status(" Passphrase: #{e[2]}") if !e[2].blank? | |
print_status(" Username: #{e[3]}") if !e[3].blank? |
For reference this is what happens with a .blank?
call on nil
object:
>> nil.blank?
=> true
>> !nil.blank?
=> false
>>
https://blog.appsignal.com/2018/09/11/differences-between-nil-empty-blank-and-present.html has more details on this if you wish to read up more about this 👍
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.
Fixed with latest updates.
Will go ahead and rebase this against the latest code changes on |
blank? should be used instead of empty?
With latest updates this should be good to go, will just wait for tests to pass before landing this. |
Release NotesThe |
blank? should be used instead of empty?. When running this script on a host which has a blank SSH key, an error is encountered: