Changed Kv.get to always return an array for recursive calls.

[PeterFCaswell]

When executing a recursive Kv.get, and there is only one item to
return, Diplomat was returning a string instead of an array of
key/value pairs. This means the caller has no idea what the key
is for the value returned.

To fix the problem, we've added a call to RestClient.return_value
that indicates that it whether this is a recurse get.

I added a spec to test the above behavoir.

One worrisome behavoir remains: if you call Kv.get() with :return
and there is no response, I'm not sure what is the proper response.
For recursive searches, I pretty sure we're still just returning an
empty string. And I think that recursive searches should always
return an array.

[scalp42]

Thanks @PeterFCaswell agreed!

[PeterFCaswell]

@scalp42 Would you do me a favor and do a close review of this code. I don't like to merge my own PR unless someone else takes a look at it.

[piavlo]

@PeterFCaswell I actually think code should always return array of hashes even then no ?recurse was specified. Current code is a hack/substitute for using ?raw api which will return the raw value and can be used for single key only.

To me the real problem i'm hitting with diplomat all the time is that Kv.get method is already too much overloaded with all the possible options and while still being too implicitly opinionated about how we want to get the results, thus we have this PR and the one I made with return_nil_values.
Another example would be where it would simply return the the array of hashes
without dropping the CreateIndex/ModiyIndex/Flags metadata.

I'd call for a Kv.get method redesign/simplify, maybe split it into several explicit methods like:

• Kv.exists? - to check if key exists , would return true/false
• Kv.get_raw - to return a key raw value , aka the ?raw
• Kv.watch - to watch for key changes , one would have to pass this method ModiyIndex
• Kv.get - to get array of keys hashes with options to return keys(dirs) with nil values and metadata (this method would not be blocking instead the watch method would take care of such needs)

Another thing is to consider dropping pre 1.9.x and lower ruby versions support which will reach it's end of life this January, so that ruby keywords could be used for better readability and usage.

[PeterFCaswell]

@piavlo I agree on all those points.
How do others feel @johnhamelink @scalp42 ?

[johnhamelink]

Yep I also agree. Only thing I'd ask is for there to be a sensible deprecation flow used so that people can migrate over easily, without breaking things for people.

On a separate note - are we due a new release of Diplomat?

[PeterFCaswell]

I'd say yes to the new release.

[piavlo]

Regarding deprecation how about something ugly but effective as naming new class Diplomat::KV
and leave Diplomat::Kv intact thus not breaking backward compatibility?

[scalp42]

I like the idea of not breaking backward compatibility for a certain time (what aws-sdk did with Aws and AWS namespace http://ruby.awsblog.com/post/TxFKSK2QJE6RPZ/Upcoming-Stable-Release-of-AWS-SDK-for-Ruby-Version-2 ) but no strong opinions.

[johnhamelink]

I think we should make the changes on a separate namespace as @piavlo mentioned, add a deprecation message to the old Kv namespace methods, and bump the version up a minor release. When we're ready to remove the old code, we can bump up to a major release and remove the dead code - that should give people who are tracking the latest releases time and fair warning to migrate over to the new syntax.

[EugenMayer]

I fiddled around quiet a bit with the needed changes here, and this is way more then expected. This patch is not sufficient and will lead to issues if you retrieve single values.

I think, before making a new namespace (which makes sense) and changing this, diplomat should completely rethink the way values of trees are gathered,e.g. the current "flattinging" instead of a real hashmap (multi-level), key-base is prefixed to all keys and more.

After this, this patch here becomes trivial. Just my 50 cents