feat:added Slack notifications support #38

wjro · 2025-05-21T23:33:29Z

wjro commented

2025-05-21 23:33:29 +00:00

No description provided.

wjro added 1 commit 2025-05-21 23:33:29 +00:00

feat:added Slack notifications support 44b2b092a8

wjro added 1 commit 2025-05-22 18:16:49 +00:00

feat: send alerts to multiple alert channels e1a8ee1c15

johnride approved these changes 2025-05-22 20:01:14 +00:00

johnride left a comment

Overall seems OK. Deserves some refactoring though.

harmony/src/modules/monitoring/discord_alert_manager.rs

						
				@@ -13,2 +9,2 @@

				        "None".to_string()

				    };

				fn get_discord_alert_manager_score(config: &KubePrometheusConfig) -> Option<HelmChartScore> {

				    let (url, name) = config.alert_channel.iter().find_map(|channel| {

johnride commented

2025-05-22 19:24:38 +00:00

Since we're supporting a list, shouldn't we use filter_map instead so we can handle all the instances at once?

Also this is a smell to me. All the handlers should implement the same trait. For example :

trait AlertEndpoint {
  fn register(&self, ...);
}

Ce serait une facon de simplifier la gestion de chacun des types ensuite. C'est plus type safe et moins error prone.

Since we're supporting a list, shouldn't we use filter_map instead so we can handle all the instances at once? Also this is a smell to me. All the handlers should implement the same trait. For example : ``` trait AlertEndpoint { fn register(&self, ...); } ``` Ce serait une facon de simplifier la gestion de chacun des types ensuite. C'est plus type safe et moins error prone.

harmony/src/modules/monitoring/kube_prometheus.rs

						
				@@ -188,2 +221,4 @@

				    }

				}

				fn discord_alert_builder(release_name: &String) -> (String, String) {

johnride commented

2025-05-22 19:58:42 +00:00

Yeah, these two functions here discord_alert_builder and slack_alert_builder here should be implementations of a trait.

The fact that their names share the same semantics is a great givaway.

Also, returning (String, String) is pretty weak. I guess there should be a type or you should build a custom type for that. Then this type can Derive Serialize.

Yeah, these two functions here `discord_alert_builder` and `slack_alert_builder` here should be implementations of a trait. The fact that their names share the same semantics is a great givaway. Also, returning (String, String) is pretty weak. I guess there should be a type or you should build a custom type for that. Then this type can Derive Serialize.

wjro merged commit 9c51040f3b into master

2025-05-22 20:04:52 +00:00

wjro referenced this issue from a commit

2025-05-22 20:04:54 +00:00

Merge pull request 'feat:added Slack notifications support' (#38) from feat/slack-notifs into master

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#38