-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Use wadm-client and wadm-types crates for wadm interactions #38
feat: Use wadm-client and wadm-types crates for wadm interactions #38
Conversation
let wadm_client = | ||
WadmClient::from_nats_client(&lattice_id, None, watcher.nats_client.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily love this, having to instantiate a new wadm_client::Client
each time we reconcile, maybe it should be possible to do something like .set_lattice(...)
with the wadm_client::Client
to switch between them, since all that currently does is generate different subjects for the various endpoints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code perspective, there really isn't a ton of overhead since it is just storing the nats client (which is a shallow clone) and the lattice string, but if there is an improvement to the client you need, let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I thinking it would be nice to store a WadmClient
instance instead of the async_nats::Client
in the Watcher
since it isn't interacting with anything other than wadm in this case, which having the ability to swap between lattices would enable.
I'm not pressed either way, it felt like not having to recreate the WadmClient every time you want to operate on a different lattice could be a nice improvement from usability perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothin' but nits
b97c924
to
ebe0c71
Compare
Signed-off-by: Joonas Bergius <joonas@cosmonic.com>
ebe0c71
to
b1d8881
Compare
Feature or Problem
Switching us from using
wash-lib
to the newly availablewadm-client
andwadm-types
crates instead, which cleans things up nicely!cc @thomastaylor312
Related Issues
Release Information
Consumer Impact
Testing
Unit Test(s)
Acceptance or Integration
Manual Verification