[CON-689] issue 1308 Elasticsearch 5 Support Will Break External Solr #1309
Conversation
Signed-off-by: Lance Finfrock <[email protected]>
| version = JSON.parse(res.body)['version']['number'].split('.').first.to_i | ||
| raise "Unsupported elasticsearch version of #{version}. There is current support for the major versions of 2 and 5." if version != 5 && version != 2 | ||
| version | ||
| rescue => e |
There was a problem hiding this comment.
[nit] Can we say StandardError here?
There was a problem hiding this comment.
It also seems really odd that we raise inside of a begin/rescue block...
There was a problem hiding this comment.
It was attempting to decorate the JSON exceptions with more context. I restructured it to be more explicit.
| version = JSON.parse(res.body)['version']['number'].split('.').first.to_i | ||
| raise "Unsupported elasticsearch version of #{version}. There is current support for the major versions of 2 and 5." if version != 5 && version != 2 | ||
| version | ||
| rescue => e |
There was a problem hiding this comment.
It also seems really odd that we raise inside of a begin/rescue block...
| raise "Unsupported elasticsearch version of #{version}. There is current support for the major versions of 2 and 5." if version != 5 && version != 2 | ||
| version | ||
| begin | ||
| version = JSON.parse(res.body)['version']['number'].split('.').first.to_i |
There was a problem hiding this comment.
It doesn't appear that we're testing the raise here when we get no data, but maybe that's for another change?
|
Thank you so much for adding tests here around external services. This is awesome! |
| @@ -104,6 +105,16 @@ Vagrant.configure("2") do |config| | |||
| attributes['vm']['elasticsearch'] = nil | |||
| end | |||
|
|
|||
| if attributes['vm'].has_key? 'external_solr' | |||
| if attributes['vm']['external_solr']['start'] == true | |||
There was a problem hiding this comment.
This would be a bit more idiomatic ruby written like:
if attributes['vm']['external_solr'] && attributes['vm']['external_solr']['start']
There was a problem hiding this comment.
It totally makes sense... but for whatever reason all the other attributes['vm'] clauses use this strange pattern. It might just be copy-pasta, but I it'd be better to just fix them all up in another pass. Some of this (the chef_backend stuff assigning nil but then reading an attribute underneath it later) doesn't even seem quite right but those codepaths may never ever get used. I might simplify it a bunch.
There was a problem hiding this comment.
This is definitely not a blocker. 👍
Signed-off-by: Kartik Null Cating-Subramanian <[email protected]>
ksubrama
force-pushed
the
lancewf/CON-689
branch
from
16ad262 to
8c365fa
Compare
| "opscode_erchef['search_provider']" => '"solr"', | ||
| "opscode_erchef['search_queue_mode']" => '"batch"' | ||
| } | ||
| json = simple_deep_merge(json, { "provisioning" => { "chef-server-config" => external_solr } }) |
There was a problem hiding this comment.
I sort of hope there is a complex_deep_merge somewhere
Signed-off-by: Lance Finfrock [email protected]
#1308