-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle requests for software updates discoveries from Host aggregate #2507
Handle requests for software updates discoveries from Host aggregate #2507
Conversation
f2e0b02
to
c64dcca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nelsonkopliku ,
The approach looks correct.
Just a couple of things:
- we don't need to use the
multi
as we don't need to get any change from previous events in this event - i think we can simply the new
maybe_
functions, solution proposed
Besides that, all good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nelsonkopliku ,
As the solution you propose is correct, I'm giving a green light.
I proposed other solution that I think is better (my opinion).
But in any case, it is ok to move forward
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nelsonkopliku ,
Good job
] ++ | ||
maybe_emit_software_updates_discovery_events( | ||
host_id, | ||
current_fully_qualified_domain_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this 2nd argument be nil
here?
I mean, I understood that in this circunstance we don't want to emit the Cleared
event, but if the new fqdn is nil
, and we had a value before (current), it would be emitted, or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I guess I missed that test.
Here we go #2518
Thanks!
Description
This PR moves the logic for software updates discovery inside the host aggregate which is going to emit a
SoftwareUpdatesDiscoveryRequested
in the following scenarios:Additionally when the fqdn of the host transitions from a meaningful value to
null
we take care of dispatching aClearSoftwareUpdatesDiscovery
for clean up purposes.Following this we will have a
SoftwareUpdatesDiscoveryEventHandler
listening toSoftwareUpdatesDiscoveryRequested
getting information from suma and dispatching back aCompleteSoftwareUpdatesDiscovery
commandWhen implementing the new event handler some minor changes to
Trento.SoftwareUpdates.Discovery
might be needed. Here's some referenceHow was this tested?
Automated tests