Issue software updates discoveries on save/change settings #2519
Conversation
nelsonkopliku
added
enhancement
nelsonkopliku
force-pushed
the
issue-software-updates-discoveries-on-save-settings
branch
2 times, most recently
from
47792ad
to
182a29e
Compare
| _ -> false | ||
| end)} | ||
| Enum.each(Hosts.get_all_hosts(), fn | ||
| %HostReadModel{id: host_id} -> |
There was a problem hiding this comment.
Do we need to go through a command/aggregate on this?
I mean, it already was doing what we need.
I understand that this change is done to have the SoftwareUpdatesDiscoveryRequested event, but honestly, I think that is better not to have these kind of events in user interactions. We discarded this for wanda for example. At the end, the event is not changing anything in the aggregate, and we are adding extra steps to the process for something that already works fine.
There was a problem hiding this comment.
To me the process is
DiscoverSoftwareUpdates -> SoftwareUpdatesDiscoveryRequested
-> event handler -> CompleteSoftwareUpdatesDiscovery (or UpdateSoftwareUpdatesDiscoveryHealth)
-> SoftwareUpdatesDiscoveryCompleted (or SoftwareUpdatesHealthUpdated/Changed)
regardless of where it is triggered.
It is just consistent. Also in the history of events.
the event is not changing anything in the aggregate
correct, for now we do not change any state.
However we're about to change software_updates_discovery_health to support also not_set and we might also have the opportunity to set the state to something like running maybe and have that information broadcasted to interested parties (the frontend for example).
Besides this the code is much simplified.
The main rationale is consistency in the process and simplification of that piece of code.
The extra opportunity for the running state is just a nice side effect.
What do you think?
There was a problem hiding this comment.
For me it just makes thinks more complex. Instead of a direct path (1 function), we use a function, command, event, event listener, function. (from command on, multiplied by the number of hosts).
All of this, just to add a event in the store, which actually I would prefer not to have it.
Imagine that you have 100 hosts, and you start saving SUMA settings from the frontend. Every single change emits 100 events. And if you fail to put the correct data in the form (which we allow), more.
For a thing, that is not a real event (from my point of view), it is just a user intervention.
More over, we had the same discussion for wanda, and decided not to add such events.
We don't necessarily need to go through the same process to reach the same goal, discover udpates.
There was a problem hiding this comment.
the problem with many hosts is that we'd need to do many requests to suma, sequentially.
Yes we could add whatever library do do concurrent things, but we'd get that concurrency for free (or almost free with an extra event in the history) by just leveraging current architecture.
There was a problem hiding this comment.
but still, I'll revert the change and see what happens
nelsonkopliku
force-pushed
the
issue-software-updates-discoveries-on-save-settings
branch
from
182a29e
to
73afdd2
Compare
@arbulu89 Thinking of addressing that in a followup PR. Would it be ok? |
nelsonkopliku
changed the title
…tes commands instead of process completion
… when saving and updating settings
nelsonkopliku
force-pushed
the
issue-software-updates-discoveries-on-save-settings
branch
from
73afdd2
to
153014a
Compare
There was a problem hiding this comment.
Hey @nelsonkopliku ,
I wouldn't personally put the discovery in the save_settings functions. At the, end save_settings should save settings. I would put it in the controller itself (which is the unique point where we do it).
Either way, the end result is the same, so I'm good with the code
| @@ -548,6 +548,12 @@ defmodule Trento.Hosts.Host do | |||
|
|
|||
| # Software Updates Discovery | |||
|
|
|||
| def execute( | |||
There was a problem hiding this comment.
We don't need this now, right?
There was a problem hiding this comment.
well does not harm and keeps away any possibility of weird things happening 😄
nelsonkopliku
deleted the
issue-software-updates-discoveries-on-save-settings
branch
Description
This PR adds the ability to trigger software updates discovery process for all the registered hosts when saving or changin settings.
It is implemented in a best effort fashion: if dispatching of
DiscoverSoftwareUpdatesfor any of the host fails (unlikely) the overall operation does not fail and saving credentials is not affected.This is needed when:
An extra clause in the host aggregate has been added that does not emit any
SoftwareUpdatesDiscoveryRequestedwhen aDiscoverSoftwareUpdatesis dispatched for a host without an fqdn (did not see yet a usecase to return an error)How was this tested?
Automated tests.