Skip to content
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(*): Notifies on manifest deployment #79

Merged

Conversation

thomastaylor312
Copy link
Contributor

Please note that this is a bit big because I had to have a stream that combined internal wadm events with wasmbus ones. However, I did realize that we only care about streams for their configuration and delivery guarantees, and not for their storage capabilities. Because of this, we don't actually have to have the users configure the streams beforehand. So while this adds one more stream to manage combining the two topics (until nats 2.10 comes out and we move to it), the user no longer has to configure any of them.

Closes #49

bin/main.rs Outdated Show resolved Hide resolved
src/events/types.rs Outdated Show resolved Hide resolved
src/mirror/mod.rs Show resolved Hide resolved
description: Some(format!("Durable wadm mirror consumer for {lattice_id}")),
ack_policy: async_nats::jetstream::consumer::AckPolicy::Explicit,
// These should be real quick to ack
ack_wait: std::time::Duration::from_secs(1),
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a mirror, it's always a local NATS message right? I'd only ask to make this configurable if this message will traverse over a real network boundary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will likely cross a network boundary. I'll bump this to 2s, but because this is temporary I don't really see a need to make it configurable

src/server/handlers.rs Show resolved Hide resolved
src/server/handlers.rs Show resolved Hide resolved
src/server/notifier.rs Outdated Show resolved Hide resolved
Please note that this is a bit big because I had to have a stream that
combined internal wadm events with wasmbus ones. However, I did realize
that we only care about streams for their configuration and delivery
guarantees, and not for their storage capabilities. Because of this,
we don't actually have to have the users configure the streams beforehand.
So while this adds one more stream to manage combining the two topics
(until nats 2.10 comes out and we move to it), the user no longer
has to configure any of them.

Closes wasmCloud#49

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312 thomastaylor312 merged commit 64d38f1 into wasmCloud:wadm_0.4 Apr 13, 2023
@thomastaylor312 thomastaylor312 deleted the feat/notify_manifests branch April 13, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants