Endpoint available software updates

[dottorblaster]

Description

Adding the endpoint to get the available software updates for a given host.

How was this tested?

Automatic tests added

[nelsonkopliku]

Here's a first round of review, well done.
We need to keep iterating tho, mainly in the result aggregation step and more thorough testing.

Keep it up!

[nelsonkopliku]

I see room for a more robust behaviour

• if host_id provided in the request is not one of the hosts in trento, we should get a 404
• given the host id we need to get the relevant fqdn to get the system id. fqdn might not be there, tho
• if we can't get a system id, because of suma failure, we do not try anything else and we should get a meaningful error (422? 503?)
• if we can get only one between relevant patches or upgradable packages, because the other fails, we could still return a partial result. But I can live also with making the whole request fail, as long as we return something meaningful

I also would encapsulate that aggregation logic in Trento.SoftwareUpdates.
Let's also add tests for failing scenarios.

[dottorblaster]

@nelsonkopliku can relate. I was actually planning to returning an empty list but this would be very silent in terms of error handling and I wouldn't like that because in the end a user might have 0 upgradable packages when actually behind the scenes it's just a failing call.

So: I like your proposal, but what do you think about an FQDN that still isn't there? Still not found?

[nelsonkopliku]

If we have the host given its id, but that host does not have an fqdn, maybe a 422 could work as well. It'd be consistent with what we often do.

If we return 404 in that case, it'd be great to know whether it is because the host is not in trento or because the host does not have an fqdn.

[nelsonkopliku]

Cool!

I guess we just need a tiny fine tune in error handling and moving aggregation logic in the relevant context. Then we're good to merge 👍

[nelsonkopliku]

A failing get_relevant_patches or get_upgradable_packages would lead to our endpoint returning a generic 500 internal_server_error.

A more graceful 422, let's say, with something like unable to retrieve software updates would give a clearer information to a client.

This aggregation logic also would perfectly fit in Trento.SoftwareUpdates, in a new function like get_host_updates/1 expecting a host id.

[jamie-suse]

LGTM, I only have a question about the absence of the from_arch & to_arch fields for upgradable packages but otherwise looks good

[dottorblaster]

@jamie-suse in the end we decided not to include those right now because they are not in the struct that the documentation shows, also I don't think we need them since we have the arch field 👍

[dottorblaster]

@nelsonkopliku I updated the fallback controller with the 422s that you requested.

About moving the aggregation from the controller to the SoftwareUpdates context, I wouldn't feel comfortable in having a function inside a given context that uses functions (like the host id fetching) coming from another context (namely Trento.Hosts).

What do you think?

[nelsonkopliku]

Not sure I understand what doesn't make you feel comfortable.
If you look at what we do in Trento.Charts for instance, it's the same.

EDIT: also Trento.Discovery using Trento.{Clusters, SapSystems} and many other cross context usage of structs as well.

[dottorblaster]

@nelsonkopliku I just applied the changes. 👍

[nelsonkopliku]

Thanks!
I was expecting to have relevant tests in Trento.SoftwareUpdates.SettingsTest 😅 .
But let's not make this go too long.

LGTM. Let's see how all the things wire up together 🚀

[nelsonkopliku]

🚀