johnride
  • Joined on 2024-02-06
johnride pushed to adr/project-delivery-automation at NationTech/harmony 2025-06-04 18:15:57 +00:00
a47c1f15fe docs: Introduce project delivery automation ADR. This is still WIP
johnride commented on pull request NationTech/harmony#48 2025-06-02 20:59:15 +00:00
feat: Initial setup for monitoring and alerting

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.

johnride suggested changes for NationTech/harmony#48 2025-06-02 17:30:28 +00:00
feat: Initial setup for monitoring and alerting
johnride commented on pull request NationTech/harmony#48 2025-06-02 17:30:28 +00:00
feat: Initial setup for monitoring and alerting

Nice 👍

johnride commented on pull request NationTech/harmony#48 2025-06-02 17:30:28 +00:00
feat: Initial setup for monitoring and alerting

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.

johnride commented on pull request NationTech/harmony#48 2025-06-02 17:30:28 +00:00
feat: Initial setup for monitoring and alerting

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.

johnride commented on pull request NationTech/harmony#48 2025-06-02 17:30:27 +00:00
feat: Initial setup for monitoring and alerting

A bit of rustdoc would be relevant here to explain the intended use of this trait.

johnride commented on pull request NationTech/harmony#48 2025-06-02 17:30:27 +00:00
feat: Initial setup for monitoring and alerting

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.

johnride suggested changes for NationTech/harmony#48 2025-06-02 12:54:31 +00:00
feat: Initial setup for monitoring and alerting
johnride commented on pull request NationTech/harmony#48 2025-06-02 12:54:31 +00:00
feat: Initial setup for monitoring and alerting

I feel like this is not a correct abstraction. NotificationAdapter has a responsibility and Deployer has another. This is a strong smell of broken Single Responsibility principle, probably interface segregation too.

johnride commented on pull request NationTech/harmony#48 2025-06-02 12:54:31 +00:00
feat: Initial setup for monitoring and alerting

I think we can solve the dependency issue by introducing a T here :

johnride commented on pull request NationTech/harmony#48 2025-06-02 12:54:31 +00:00
feat: Initial setup for monitoring and alerting

SRP problem

johnride commented on pull request NationTech/harmony#48 2025-06-02 12:54:30 +00:00
feat: Initial setup for monitoring and alerting

Violates OCP : we want to allow external contributors to provide new backend implementations (that is, if we want to introduce the concept of monitoring backend right now or wait until the needs arises?)

johnride created pull request NationTech/harmony#47 2025-05-30 13:15:13 +00:00
TenantManager_impl_k8s_anywhere
johnride deleted branch feat/tenantScore from NationTech/harmony 2025-05-30 13:13:55 +00:00
johnride pushed to master at NationTech/harmony 2025-05-30 13:13:52 +00:00
60f2f31d6c feat: Add TenantScore and TenantInterpret (#45)
johnride merged pull request NationTech/harmony#45 2025-05-30 13:13:50 +00:00
feat: Add TenantScore and TenantInterpret
johnride deleted branch TenantManager_impl_k8s_anywhere from NationTech/harmony 2025-05-29 20:15:53 +00:00
johnride pushed to master at NationTech/harmony 2025-05-29 20:15:48 +00:00
27f1a9dbdd feat: add more to the tenantmanager k8s impl (#46)