cleanup joyent.rb #499
cleanup joyent.rb #499
Conversation
bahamas10
commented
Mar 17, 2015
bahamas10
commented
- condense code
- touch up style
- don't stat(2) before open(2)
|
This all looks sane to me 👍 @chef/client-core |
| else | ||
| nil | ||
| end | ||
| ::File.read("/opt/local/etc/pkg_install.conf").strip.split("=", 2).last rescue nil |
There was a problem hiding this comment.
Can we make this rescue more specific? The way it's written now we'll rescue anything in .strip.split("=", 2).last when really we just want to rescue ENOENT and EACCES on the file read.
There was a problem hiding this comment.
@danielsdeleo is it proper for an ohai plugin to have an uncaught exception? i figured the goal was to avoid that
There was a problem hiding this comment.
Ohai catches all exceptions at a higher layer, which I dislike but we can't change that now. So any uncaught exception in a plugin will be rescued and cause a debug message to be printed, so if this plugin were to fail because, say, we had unexpected content in /opt/local/etc/pkg_install.conf, at least you would be able to get some feedback about why you suddenly didn't get the ohai data you expected. Which illustrates why I prefer not to do "blind" rescues like this (except in special cases), it hides or obfuscates the true source of the problem when something goes wrong.
There was a problem hiding this comment.
ah ok, i wasn't aware of ohai capturing all exceptions.
would it be best to remove the rescue statement completely then? I think it's just as much of a problem if /opt/local/etc/pkg_install.conf doesn't exist, as it is if it contains garbage data.
There was a problem hiding this comment.
You'd be the person to know that better than me. Are we on a fundamentally broken system if /opt/local/etc/pkg_install.conf doesn't exist? Or is it a normal thing?
There was a problem hiding this comment.
ah good point, i'll make changes and push up a new commit.
(no, the system is not fundamentally broken in that case, so i'll modify the code to reflect that)
- condense code - touch up style - don't stat(2) before open(2)
| lines << line | ||
| end | ||
| end | ||
| data = ::File.read("/etc/product") rescue nil |
There was a problem hiding this comment.
You'll need to stub ::File.read("/etc/product") in the specs. The failures appear to be due to this line returning nil.
| if data | ||
| data.strip.split("\n") | ||
| else | ||
| nil |
There was a problem hiding this comment.
if returns nil if the condition is false.
thommay
added
Expeditor: Skip Version Bump
