-
Notifications
You must be signed in to change notification settings - Fork 93
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
Webhook notifications #884
base: master
Are you sure you want to change the base?
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.
Nice work so far. The way you've written the code is easy to follow and understand, so I don't have many comments on the code for now (this is a good thing).
I see you injected the Notifier into the deals module. That is a fine place to start, but I'm guessing there will be a different place we may want to leverage the Notifier, or at least an additional place or two.
If I'm remembering things correctly, the Scheduler will be a very good place to use Notifier. The Scheduler manages all things related to storing, retrieving, renewing and repairing deals. For this reason, it will be a source for many notifications. I'd imagine those events might be higher level with less detailed information, so maybe we augment those notifications with finer grained notifications from the deals module or any other module that makes sense.
So, sounds like we should dig into the Scheduler and understand how it works and how notifications fit in.
|
||
type WebhookAlert struct { | ||
Type string | ||
Threshold string |
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.
Curious to see what you're thinking for Threshold
use cases. Sounds interesting.
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.
Coming along really nicely. It seems like everywhere you're integrating the notifier into the system makes sense to me. I like the implementation of the notifier and the way you define notification types, process events, and calculate notification to send. Nice work.
@@ -38,6 +38,7 @@ type RetrievalAction struct { | |||
Miners []string | |||
WalletAddress string | |||
MaxPrice uint64 | |||
Notifications []*ffs.NotificationConfig |
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'm curious why this addition is only in RetreivalAction
and not StorageAction
, but maybe I'll learn this later in my review.
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.
StorageAction
already contains ffs.StorageConfig
, which has Notifications []*NotificationConfig
type configStore struct { | ||
sync.RWMutex | ||
|
||
configs map[ffs.JobID][]*ffs.NotificationConfig | ||
} |
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.
Will this need to write to a persistent store (database) at some point?
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.
yes, I think at some point would be better to put it to persistent store, for the beginning I guess we can keep it in memory
depends_on: | ||
- ipfs | ||
- lotus | ||
environment: | ||
- POWD_DEVNET=true | ||
- POWD_LOTUSHOST=/dns4/lotus/tcp/7777 | ||
- POWD_IPFSAPIADDR=/dns4/ipfs/tcp/5001 | ||
- POW_GATEWAYHOSTADDR=0.0.0.0:7001 |
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.
Just curious why these changes popped up (and in docker-compose.yaml
as well). Maybe something you needed for local testing? If it's generally useful, feel free to keep the changes.
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.
yes, it's just for local testing, would need to revert it back
when I tested locally found that on my laptop some battery management uses this port :)
Draft