Skip to content
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

Software updates event handler #2515

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Apr 12, 2024

Description

This PR introduces a SoftwareUpdatesDiscoveryEventHandler reacting to SoftwareUpdatesDiscoveryRequested events, getting information from SUMA and dispatching back in the system a CompleteSoftwareUpdatesDiscovery possibly resulting in a host's health change.

What's next:

  • make the discovery tick dispatch DiscoverSoftwareUpdates command so that SoftwareUpdatesDiscoveryRequested + event listener is triggered (currently we'd only have a completion event without an initiation event)
  • trigger software updates discovery process for all the registered hosts when saving/changing suma settings
  • improve consistency between health in hosts list and what gets returned in GET /hosts/:id/software_updates

How was this tested?

Automated tests.

@nelsonkopliku nelsonkopliku self-assigned this Apr 12, 2024
@nelsonkopliku nelsonkopliku added the enhancement New feature or request label Apr 12, 2024
@nelsonkopliku nelsonkopliku force-pushed the software-updates-event-handler branch 3 times, most recently from 17a46e6 to 11b386d Compare April 12, 2024 14:04
@nelsonkopliku nelsonkopliku marked this pull request as ready for review April 12, 2024 14:14
@nelsonkopliku nelsonkopliku force-pushed the software-updates-event-handler branch from 11b386d to ee4c44b Compare April 15, 2024 08:16
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!
I dropped some cosmetic related comments mostly.

PD: I commented on the option of handling some errors in the discovery. It is true that it would just be boilerplate code.
I was just wondering how we will notify the user when a discovery fails, because until now, we just print some messages in the logs.

I guess this discussion goes beyond of this PR. Would it make sense to have a discussion about it?

lib/trento/software_updates/discovery.ex Outdated Show resolved Hide resolved
lib/trento/software_updates/discovery.ex Outdated Show resolved Hide resolved
lib/trento/software_updates/discovery.ex Outdated Show resolved Hide resolved
) do
Discovery.discover_host_software_updates(host_id, fully_qualified_domain_name)

:ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find this always :ok return not really safe or robust.
I would personally check what the discovery returns, and based on that return some error.

We can implement the error function later on, even if it does a simple :skip.
This would give the look that we are dealing with the error (even though initially is doing nothing).

Maybe in the future, we could have some retry strategy, or broadcast some message to the frontend, or anything like that.

I find it a elegant solution at least.

PD: The error function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, as we are not going to do anything with the failure by now, I'm okey to leaving as it is.
Maybe just putting a comment like The error is not being handled, as there is not any strategy for it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, some more robust error handling might be beneficial.
I just wanted to get the flow and iterate on errors as per the needs

@nelsonkopliku nelsonkopliku force-pushed the software-updates-event-handler branch from 93bd658 to d239d48 Compare April 15, 2024 09:29
@nelsonkopliku nelsonkopliku added the env Create an ephimeral environment for the pr branch label Apr 15, 2024
@nelsonkopliku nelsonkopliku merged commit 3b605d4 into main Apr 15, 2024
23 checks passed
@nelsonkopliku nelsonkopliku deleted the software-updates-event-handler branch April 15, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request env Create an ephimeral environment for the pr branch
Development

Successfully merging this pull request may close these issues.

3 participants