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

Avoid SUMA health complete on settings not configured error #2547

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

arbulu89
Copy link
Contributor

Description

Avoid sending CompleteSoftwareUpdatesDiscovery during the discovery if the SUMA settings are not configured.
This is needed in scenarios like when the SoftwareUpdatesDiscoveryEventHandler handler runs the discovery.
Without this change, the health of the software updates is set to unknown, even though we shouldn't be changing it, as SUMA settings are not configured.

So the unique difference is that the settings_not_configured error doesn't dispatch the command, nothing else.

How was this tested?

UT added and tested manually

@arbulu89 arbulu89 added bug Something isn't working enhancement New feature or request labels Apr 24, 2024
@arbulu89 arbulu89 marked this pull request as ready for review April 24, 2024 06:47
@arbulu89 arbulu89 force-pushed the avoid-suma-health-change-on-settings-not-configured branch from 5adc348 to a10eac6 Compare April 24, 2024 06:49
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Thanks @arbulu89.
Have you considered implementing a command middleware doing this check every time a CompleteSoftwareUpdatesDiscovery is dispatched?

I would have implemented it that way, however I can live with this as well.

@arbulu89
Copy link
Contributor Author

@nelsonkopliku No, I didn't consider that.
How would you do that? Querying the database every time we have this command?
It looks like less efficient. As we already have the value, why not just simply use it?

@arbulu89 arbulu89 added the env Create an ephimeral environment for the pr branch label Apr 24, 2024
@nelsonkopliku
Copy link
Member

nelsonkopliku commented Apr 24, 2024

Yes, maybe some extra tiny query when that command would be dispatched 🙈

Your proposed solution is just fine, at the end when that command gets dispatched a discovery (read attempt to call suma) have already happened, so we wouldn't be blocking anything interesting in such a middleware.

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@arbulu89 arbulu89 merged commit c035fab into main Apr 24, 2024
50 of 51 checks passed
@arbulu89 arbulu89 deleted the avoid-suma-health-change-on-settings-not-configured branch April 24, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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