31e59937dc
Merge pull request 'feat: Initial setup for monitoring and alerting' (#48) from feat/monitor into master
0d56fbc09d
wip: applied comments in pr, changed naming of AlertChannel to AlertReceiver and added rust doc to Monitor for clarity
Does this even work? I don't see the binding with the ensure_ready impementation of K8sAnywhere.
autoloading inventory here is a big smell, you should avoid this as much as possible. What if the used built a custom Inventory and now you're autoloading his production inventory and you start wiping operating systems and network configurations?
This function looks really weird, I feel like this is a hack to link the config and the dependency together.
This should not be called deploy as this will not deploy every time it is called. We use "ensure" for this behavior.
It's a bit weird to use a public raw function like this to initialize a Score, why not just use the usual struct building method?
This rustdoc is pretty good to explain the abstraction, but giving a concrete example such as Prometheus would make a complete understanding much easier. Even I still have a bit of trouble to figure it out as-is.
You don't have to specify Topology here. I think it's better to keep the trait bindings to the minimum. DiscordWebhookSender should be enough.
I think this trait is correct but the way it is implemented feels wrong. I understand that you currently only have the discord implementation and you interact directly with it. But I feel like there will be a use case where we will want to have simple getter and setters for common attributes of AlertChannelConfig. I don't see a 100% clear use case right now though.
I don't think webhook url deserves to be in this abstract trait. It is possible that the AlertChannel does not have a webhook, such as SMS, phone calls, SMTP.
A bit of rustdoc would be relevant here to explain the intended use of this trait.
I think we can solve the dependency issue by introducing a T here :