Add support for Softlayer cloud #448
Conversation
| # true:: If the softlayer cloud can be identified | ||
| # false:: Otherwise | ||
| def looks_like_softlayer? | ||
| !!hint?('softlayer') |
There was a problem hiding this comment.
I'm a little confused on the need for a !!. Looking through the hints code I see that it will either return a hash or nil(which will evaluate to false). Can you just use hint?('softlayer')
Also it would be great to leverage the mixin for softlayer in order to see if you can hit the api url. Then looks_like_softlayer? can be expanded to
hint?('softlayer') || has_softlayer_metadata?
There was a problem hiding this comment.
Yeah, I'm not sure why this is really necessary.
| 'instance_id' => fetch_metadata_item("getId.txt") | ||
| } | ||
|
|
||
| metadata |
There was a problem hiding this comment.
this line is useless in ruby.
lamont-granquist
added
Waiting for Contributor Response
and removed
Pending Maintainer Review
labels
smurawski
added
Pending Maintainer Review
and removed
Waiting for Contributor Response
labels
|
This might need a rebase due to some other previous updates to master. |
|
ping @akarpik can you rebase and resubmit? |
| @@ -11,7 +11,7 @@ gem "rspec_junit_formatter", :git => 'https://github.com/sj26/rspec_junit_format | |||
| group :development do | |||
| gem "chef", github: "opscode/chef", branch: "master" | |||
|
|
|||
| gem "sigar", :platform => "ruby" | |||
| # gem "sigar", :platform => "ruby" | |||
There was a problem hiding this comment.
looks like something edited out in testing that should be dropped?
|
@chef/client-core @chef/client-engineers i sorta feel strongly that this is still incorrect behavior with the empty hash for softlayer attrs if the metadata service returns an error. the kinds of conditions that i'm worried about are now we don't get our ip addresses in the metadata and the chef run continues successfully and then the node is posted and now searches for the node will not have the ip address. it would be better to allow the error here. it would be even better to be able to abort ohai and abort the wrapping chef run completely. need some more thoughts on resolving this. |
|
I think for now it should be consistent with what the other plugins do. With Ohai 7 we wanted to implement better behavior for error handling (read: don't just swallow failures) and also some way of failing the chef run if certain attributes didn't get set in ohai, but ran out of time. We're a lot closer with the Ohai 7 architecture though. |
|
👍 for consistency, as long as it's something that does eventually get fixed |
smurawski
added
Pending Maintainer Review
and removed
Waiting for Contributor Response
labels
|
@akarpik We understand the use case for wanting to leave the softlayer node as a hint, but that is not the behavior of existing plugins and the direction we want to go with Ohai includes adding support to bubble up errors and deal with them in a consistent manner. This would add an exception to the current use case and probably break moving forward. If you remove that behavior, we can merge in the additional softlayer metadata changes, otherwise we'll have to close the PR. |
|
@smurawski, I will remove behaviour, and update tests. |
|
@akarpik Thank you! Looking forward to getting your softlayer support merged! |
|
how's this looking now @smurawski |
mcquin
added
Waiting for Contributor Response
and removed
Pending Maintainer Review
labels
mcquin
added
Pending Maintainer Review
and removed
Waiting for Contributor Response
labels
|
👍 when this is merged we'll need to be careful about the changelog. |
|
cc @chef/client-core for one more review. |
|
👍 |
Add support for Softlayer cloud
thommay
added
Status: Pending Maintainer Review
and removed
Pending Maintainer Review
labels
Add support for Softlayer cloud