Handle software updates discovery errors #2532
Conversation
…ftware_updates_discovery_health goes unknown
nelsonkopliku
force-pushed
the
handle-software-updates-discovery-errors
branch
from
f930a0d to
0307ce6
Compare
nelsonkopliku
added
enhancement
There was a problem hiding this comment.
Hey @nelsonkopliku ,
I find the change overly complex.
Why not simply dispatch the command and returning the error?
In fact, the dispatch error case looks a bit over defensive. Our system won't send this command to any host that is not already registered.
I mean, if in the unknown state, if we receive host_not_registered (I don't know when), who cares, we still need to return the original error. At the end, we just want to know if there was an error or not. We don't use the specifics anywhere. I mean, what use has this dispatching error (that why the way, you don't send with the same format for the correct flow of it fails)?
| Logger.error( | ||
| "An error occurred during software updates discovery for host #{host_id}: #{inspect(error)}" | ||
| ) | ||
|
|
||
| {:error, error} | ||
| dispatch_completion_command_on_discovery_error(host_id, discovery_error) |
There was a problem hiding this comment.
I don't understand a thing 😅
Wasn't as simple as putting the the dispatch with the unknown state in the else side?
commanded().dispatch(
CompleteSoftwareUpdatesDiscovery.new!(%{
host_id: host_id,
health: SoftwareUpdatesHealth.unknown()
})
)
error
There was a problem hiding this comment.
that's where I started from, honestly.
I wanted to avoid dispatching a CompleteSoftwareUpdatesDiscovery command with unknown when the failure was not a discovery failure, but an error in dispatching CompleteSoftwareUpdatesDiscovery after a successful discovery, as mentioned in the description.
I'd be fine also with that dispatch in the else side 🙈
|
@arbulu89 thanks for the review, I applied the simplified version. |
Description
This PR handles software updates discovery errors by dispatching an
unknownhealth.Cleaned a bit up error propagation.
How was this tested?
Automated tests.