NationTech/harmony
5
0
Fork 2
Code Issues 37 Pull Requests 16 Actions Packages Projects Releases 1 Wiki Activity

refactor: Remove InterpretStatus/Error & Outcome from Topology #99

Merged
letian merged 1 commits from remove-interpret-status-from-topology into master 2025-08-09 22:52:26 +00:00
Conversation 1 Commits 1 Files Changed 15 +361 -141
letian commented 2025-08-07 02:42:23 +00:00
Owner
Copy Link

The InterpretStatus, InterpretError, and Outcome leaked in places it shouldn't have: the Topology. Because of this, refactoring to better track & instrument Interpret was more difficult as it might have effects on the Topology as well. Which wasn't a desirable outcome.

This PR introduces a TopologyState along with its TopologyStatus, and a PreparationOutcome along with its PreparationError.

A clear separation was introduced between the TopologyState/TopologyStatus and the PreparationOutcome/PrepareError because they serve different purposes/were needed by different actors.

TopologyState/TopologyStatus serves only the Maestro to track the Topology state (whether it's ready or not). Whereas the PreparationOutcome/PreparationError serves the Topology itself for every individual actions it might take in order to be ready (e.g. installing k8s client, configuring the k8s tenant manager, installing prometheus, etc.).

For now, I couldn't find an elegant way to move the TopologyState management inside the topology itself without pushing this responsibility back to the implementers of each Topology. So that's why it's currently handled by the Maestro itself. Which in a way makes sense as it's part of its responsibilities.

Sidequest (sorry 😅): also replace the multiple Topology events by a single TopologyStateChanged event. We will probably do the same for the Interpret events later on.

The `InterpretStatus`, `InterpretError`, and `Outcome` leaked in places it shouldn't have: the `Topology`. Because of this, refactoring to better track & instrument `Interpret` was more difficult as it might have effects on the `Topology` as well. Which wasn't a desirable outcome. This PR introduces a `TopologyState` along with its `TopologyStatus`, and a `PreparationOutcome` along with its `PreparationError`. A clear separation was introduced between the `TopologyState`/`TopologyStatus` and the `PreparationOutcome`/`PrepareError` because they serve different purposes/were needed by different actors. `TopologyState`/`TopologyStatus` serves only the `Maestro` to track the `Topology` state (whether it's ready or not). Whereas the `PreparationOutcome`/`PreparationError` serves the `Topology` itself for every individual actions it might take in order to be ready (e.g. installing k8s client, configuring the k8s tenant manager, installing prometheus, etc.). For now, I couldn't find an elegant way to move the `TopologyState` management inside the topology itself without pushing this responsibility back to the implementers of each `Topology`. So that's why it's currently handled by the `Maestro` itself. Which in a way makes sense as it's part of its responsibilities. --- Sidequest (sorry 😅): also replace the multiple Topology events by a single `TopologyStateChanged` event. We will probably do the same for the Interpret events later on.
letian added 1 commit 2025-08-07 02:42:26 +00:00
refactor: Remove InterpretStatus/Error & Outcome from Topology
Some checks failed
Run Check Script / check (pull_request) Has been cancelled
Details
f876b5e67b
johnride approved these changes 2025-08-08 17:13:15 +00:00
johnride left a comment
Owner
Copy Link

LGTM 🎉

LGTM 🎉
harmony/src/domain/topology/mod.rs
@@ -63,0 +73,4 @@
}
#[derive(Debug, Clone, new)]
pub struct PreparationError {
johnride commented 2025-08-08 17:13:08 +00:00
Owner
Copy Link

I love this! Feels natural and clean.

Also very naturally outlines a trend to eventually avoid the boilerplate of having each module define its own set of Outcome, Error. We will then refactor with a standardized approach when time comes.

I love this! Feels natural and clean. Also very naturally outlines a trend to eventually avoid the boilerplate of having each module define its own set of <X>Outcome, <X>Error. We will then refactor with a standardized approach when time comes.
👍 1
letian merged commit dcf8335240 into master 2025-08-09 22:52:26 +00:00
letian deleted branch remove-interpret-status-from-topology 2025-08-09 22:52:29 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
johnride
Labels
Clear labels
Architecture Decision Record
Good first issue
RFC
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
johnride jvtrudel letian stremblay wjro
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: NationTech/harmony#99
No description provided.
Delete Branch "remove-interpret-status-from-topology"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?