-
Notifications
You must be signed in to change notification settings - Fork 255
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
Add outbound MQTT factor #2722
Add outbound MQTT factor #2722
Conversation
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 think this is a good start. I won't block on the change from the closure trait object to using a custom trait instead, but I do think it's something we probably should do eventually.
use spin_world::v2::mqtt::{self as v2, Connection, Error, Qos}; | ||
use tracing::{instrument, Level}; | ||
|
||
pub type CreateClient = Arc< |
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.
Looking at the other factors, we've normally been handling this pattern with custom trait trait objects rather than closure trait objects. So instead of this type alias we would do something like:
trait ClientCreator {
fn create(
address: String,
username: String,
password: String,
keep_alive_interval: Duration,
)
}
// ...
impl InstanceState {
pub fn new(allowed_hosts: OutboundAllowedHosts, create_client: Box<dyn ClientCreator>) {
// ...
pub allowed_hosts: OutboundAllowedHosts, | ||
pub connections: table::Table<Box<dyn MqttClient>>, | ||
pub create_client: CreateClient, |
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.
We've been a little inconsistent about this but I wouldn't (necessarily) default to making these pub
, which implies that they are part of the factor's public interface.
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 agree - in the spirit of trying to treat these as reusable libraries, we should be as careful as possible about only exposing the absolute minimum we need.
dyn Fn(String, String, String, Duration) -> Result<Box<dyn MqttClient>, Error> + Send + Sync, | ||
>; | ||
#[async_trait] | ||
pub trait ClientCreator: Send + Sync { |
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.
Let's not forgot documentation here! We can do that in a follow up though.
Co-authored-by: rylev <ryan.levick@fermyon.com> Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
I rebased... |
This PR adds the outbound-mqtt factor along with the associated tests.