Allow for conditional notifications with new Notify API

[eikenb]

Updates the Notifier interface, and the implementations, with the new version of the Notify() method signature. The new version takes the newly updated data as the argument and returns a boolean that determines if Wait() returns or continues to wait. This, along with how it calls the existing template or otherwise manages notification updates, gives the Notify() implementer control over whether that update triggers an action (like template rendering) in the applications.

Fixes #27

[eikenb]

Broken up into 4 commits. First one contains the main changes. Last couple add a doc_test example w/ a KV filtered notifier. Remaining commit is just all the testing tweaks to work with API changes.

[eikenb]

One thing I'd like feedback on is the KvValue field. I thought having something other than just the bare string type for type asserting would be useful and so added only that as simply as I could. But I was also considering making it a bit more detailed and have it return either a locally defined KvPair with at least the key and value stored or we could use the original, underlying Consul KVPair struct that has a lot more fields on it.

If we extend the type to have more information, I'd also welcome feedback on the either/or pairing above, the local vs original options, as this would guide my designs for the rest of these types.

1. wrap the original data in custom types
   • gives us more control
   • only need to add new fields as this is what is currently in place
2. use the original data from the server's API (from Consul, Vault, etc.)
   • would always be up to date with new fields added by the API
   • but would make our data be completely reliant on the API library
   • could link out to original docs

[lornasong]

Excited about this change! I found the KvValue type and example usage really, really helpful. From looking over, the changes make sense to me. I'd like to spend some time playing around and making sure I really understand everything before resume reviewing if that's ok. Planning to do this later on today and maybe into Monday. Let me know if I start blocking you!

Regarding feedback on KvValue, I appreciate the analysis you provided! 🤔 I currently lean towards your PR's implementation of KvValue as a string type (I'll call it option 0) or option 1.

Thinking about option 2, it looks like the Read Key API response provides a lot of additional fields that I think hcat users are unlikely to need e.g. CreateIndex, Session, Flags etc. In the Read Key API, the fields (in addition to 'value') that I think might be useful are key and namespace; however I think it's likely that a user will already have this info if they care about them. e.g. a user will definitely already have the key; if they care about namespace, they'll have the namespace to query Read Key API. So I currently lean towards returning only what was requested (option 0) or only adding new fields that seem useful in a custom type (option 1). To be fair, I'm not sure if this can be abstracted to all APIs and might be thinking about this too narrowly since I don't have a good understanding of use cases!

[eikenb]

Thanks for the feedback @lornasong, please take your time and review it on your schedule. I have plenty to do.

The main thought that crossed my mind while working on the KV/notifier example was how you could only do it for 1 KV field. With KvValue as a typed string you can only use the string value returned in the Notify logic as there is no way for you to know which key if there was >1 in the template.

That is why I included idea of the more complex type and for option 1.

I thought about option 2 then as I'm not sure if KV is the only field where this might come up or that the Key field is the only one that'd prove to be relevant (eg. like Namespace). Option 2 simply elides out this issue by passing along everything it has. So if there was additional fields that could matter, they'd be there.

Hope the additional context helps. Thanks for looking and, again, please take your time.

[lornasong]

Thanks @eikenb! And thank you for explaining and the additional context. That's a good point about >1 KV in the template! Definitely sounds like Option 1 or 2 would be best (rather than 0). I'll have to also do some rethinking on Option 2 🤔

[eikenb]

There are 4 template functions that currently receive strings or string slices from the dependency query object's fetch methods. So these would all probably benefit from a bit more form.

Thinking about option 1 vs 2... Comparing a few of the data structures I see they are mostly exact copies or copies with a few fields missing and the missing fields were added to the API after the local struct was added (ie. they weren't there when the initial copy was made). So it doesn't see they were added to eliminate fields. That is I haven't really figured out why they were added and the git commit messages from when they were added don't shed any light on it.

The trade-off at this point seems to be all fields always kept up to date vs. requiring the extra API library imports when you need to work with those types.

I'm mainly trying to decide if 2)'s downside of requiring the API libraries imported will be an issue.

[lornasong]

That’s interesting that the missing fields seem like they may be omitted due to lack of up-keep!

Is the extra API library that you’re considering importing a different library from the one that has the consul client to make the API requests? I think I might be misunderstanding - it looks like, for example, the api library’s KvPair{} response object is in the same library as the client.KV().Get(). In order to keep option 1 updated with the latest field, would hcat still be reliant on keeping updated with the API library?

In ESM we import CheckRunner from the Consul, which made any ESM customizations (e.g. additional http request configurations) more difficult to add. That’s the only downside I’ve personally experienced so far with importing Consul rather than writing our own. 🤔 I’m don’t think it applies as much to this case though since we’re using the Consul api client.

[eikenb]

I am thinking of the standard Consul/Vault API libraries, nothing more. It probably isn't a big deal as they are indirect dependencies anyways, so you'd be pulling them either way.

I'm leaning toward just using the types as they are returned by the APIs directly. I'm having a hard time coming up with a down side to this... which is good, but it seems too easy.

[lornasong]

👍 I too am leaning towards Option 2 and using the types returned by the APIs directly. Agree - I also can't think of a down-side but feel something weird about it.

[eikenb]

Ok. Here's the plan then.

I'm going to merge this as is. Then I'm going to put together a new pull request with option 2. This will give us a chance to see it in action and hopefully relieve our hesitation. Sound good?

[lornasong]

Yes! Sounds good. Thanks for the discussion :). I'll move forward and consume the Notify() changes in CTS, and we can take our time to figure out how the option plays out!