feat/discover_inventory #127
No reviewers
Labels
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: NationTech/harmony#127
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "feat/discover_inventory"
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?
Fully automated inventory gathering now works!!
Been waiting a long time for this feature
Boot up harmony_inventory_agent with
cargo run -p harmony_inventory_agentLaunch the DiscoverInventoryAgentScore , currently available this way :
RUST_LOG=info cargo run -p example-cli -- -f Discover -yAnd you will have automatically all hosts saved to the database. Run
cargo sqlx setupif you have not done it yet.@ -35,3 +34,4 @@}}pub fn summary(&self) -> String {A garder un oeil si on aurait besoin de presenter ce summary de manieres differentes (e.g. fichier json). A ce moment-la, il pourrait etre interessant d'y aller avec le pattern de "presenter":
@ -2,0 +11,4 @@impl InventoryRepositoryFactory {pub async fn build() -> Result<Box<dyn InventoryRepository>, RepoError> {Ok(Box::new(SqliteInventoryRepository::new(&(*DATABASE_URL)).await?,Just sharing a thought even if it's not the most representative situation here (considering we're in the infra). Though it still indirectly highlights the underlying issue.
I understand the simplicity of having a
configcrate and using it when needed, but it actually makes things harder to reuse/test in isolation. All of this because in the end we depend on an env var that forces anyone using theharmonycrate to use that as config mechanism instead of other options.It's fine to use this
configcrate in theCLI(orTUIor any frontend), but for the actualharmonylib it would be a better practice to simply pass the params in the functions.Now here, we're kinda "forced" to do this because the
InventoryRepositoryis created in the middle of the execution of anInterpret. It would have been better to properly inject it, but that would mean itsScorewon't be stateless anymore (as it would be our only injection mechanism as of now).I'm not saying we should fix this now because this is a bigger architectural issue, but we should keep an eye on it and tackle this sooner than later.
Extra note: despite the appearances using a static factory doesn't really save us from the OCP violation that we would introduce if we were directly instantiating the concrete class. Yes we can switch to another instance without modifying existing code, but it will force everyone using it to switch as well. Which might not be expected and will make it harder in the end to fix in a bigger codebase (it leaves scars 😢). Again, it's ok here, but we should be very careful about using this "pattern" (it's actually an anti-pattern).
I'll create an issue about this dependency injection problem we have in our
Interprets(and at some extent theScoresstatelessness).Yeah the factory doesn't bring much value here, no time to do better but at least it allows a minimum of decoupling if we want to switch repository type for the entire application instance, or provide more methods that do take parameters and can apply additional logic when building the repo.
@ -16,3 +16,3 @@impl SqliteInventoryRepository {pub async fn new(database_url: &str) -> Result<Self, RepoError> {let pool = SqlitePool::connect(database_url)let _pool = SqlitePool::connect(database_url)poolis used so we shouldn't need the_@ -53,0 +78,4 @@.unwrap();trace!("Found host information {host:?}");// TODO its useless to have two distinct host types but requires a bit muchIt seems useless but I'm not sure they should actually be merged: to me it feels like two different concepts with a different lifecycle. It is highlighted by the fact that the
harmony_inventory_agentis a standalone service that could be consumed by other programs and that thisinventorymodule here is just one of them. Theinventoryhas its own representation of aPhysicalHostthat might become different than the one from theharmony_inventory_agent. At least it's my hypothesis for now.@ -53,0 +71,4 @@info!("Getting host inventory on service at {address} port {port}");tokio::task::spawn(async move {info!("Getting inventory for host {address} {port}");This log and the one above are very similar
@ -58,1 +124,4 @@status: InterpretStatus::SUCCESS,message: "Discovery process completed successfully".to_string(),})}There's an edge case here that might break this score:
discover_agentsis awaited, but the spawned task inside the callback are not awaited at all. So in some edge cases,discover_agentsmight finish executing before the task itself.And because we don't await for the task inside the callback, error handling is a bit awkward. Currently it says "all good" even though there could be an error: