Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Fix KVGetResult scheme to match Consul API #13

Merged
merged 2 commits into from
Nov 18, 2018

Conversation

ppressives
Copy link
Contributor

Actually Value field in Consul API is optional and deserialization fails if key value is null.

curl -X PUT localhost:8500/v1/kv/foo/
true
curl localhost:8500/v1/kv/foo/

[
    {
        "LockIndex": 0,
        "Key": "foo/",
        "Flags": 0,
        "Value": null,
        "CreateIndex": 21,
        "ModifyIndex": 21
    }
]

@ppressives
Copy link
Contributor Author

ppressives commented Jul 16, 2018

@rossabaker

Copy link
Contributor

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the slow response. I took a vacation, and then my motherboard took a vacation after that.

@@ -5,7 +5,7 @@ import argonaut._, Argonaut._
/** Case class representing the response to a KV "Read Key" API call to Consul */
final case class KVGetResult(
key: String,
value: String,
value: Option[String],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a binary breaking change and require another major version, though I can't think of a better way. It would probably be incorrect and/or evil to conflate the empty string with a null value here.

@aldiyen, do you have any thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh interesting, the Consul documentation doesn't seem to say anything about that. I cannot think of any better alternative either.

I think either kvGetRaw or kvGetJson needs to change as well to handle the null value in Consul's KV store situation, as I suspect that kvGetRaw will return Some(Array()) and then the JSON parsing will blow up, which seems more confusing than having kvGetJson spit out a None. I haven't tested this theory though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right. Corrected this case.

@aldiyen
Copy link
Contributor

aldiyen commented Nov 15, 2018

This looks like it'll work to me.

@rossabaker
Copy link
Contributor

I feel terrible for the delay on this. Please don't hesitate to give me a nudge if I'm this unresponsive.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants