Use embeds software updates #3275
Conversation
arbulu89
added
enhancement
| @@ -59,7 +59,7 @@ defmodule Trento.Infrastructure.SoftwareUpdates.MockSuma do | |||
| to_version: "1.16.2", | |||
| to_release: "1", | |||
| to_epoch: "0", | |||
| to_package_id: 92_348_112_636 | |||
| to_package_id: "92348112636" | |||
There was a problem hiding this comment.
This change will require to reset SUMA values in demo, as using an integer was not expected
There was a problem hiding this comment.
@nelsonkopliku @janvhs Do you know why we moved this value to integer in the past?
Here: #2916
The to_package_id value is defined as string in the SUMA docs: https://documentation.suse.com/suma/4.3/api/suse-manager/api/system.html#apidoc-system-listLatestUpgradablePackages-loggedInUser-sid
With that change, if the value is really a string I don't really know how this worked in the first place...
And if it is an number, our backend code was wrong.
There was a problem hiding this comment.
not sure why it became an integer. In other places is considered a string, as it seems it should be.
What happens if you just put back to strings in the mock definition (the only place in the backend where it seems to be an integer)?
There was a problem hiding this comment.
Well, it works with my mocked data. I was wondering if the SUMA docs have some typo.
I will start a SUMA instance and try myself.
The code with different types is suspicious
There was a problem hiding this comment.
True, it looks like suma is actually returning an integer, at least on our testing instance running on 4.3
There was a problem hiding this comment.
Yes. I'm going to change the field type to integer. Now everything works with a real SUMA instance.
I asked the SUMA team as well.
I hope it is only a documentation error, otherwise we might need to do changes if they fix it
arbulu89
force-pushed
the
use-embeds-software-updates
branch
from
e42e034 to
0e6b693
Compare
|
The preview environment for this pull request is ready at 3275.prenv.trento.suse.com. |
|
The API bc check is failing due the change, but this is actually normal, as we fix an error with this. |
Description
Refactor SUMA software updates discovered struct to use known fields.
All of this started with the intention to remove the
Jason.decode!(keys: :atoms)in https://github.com/trento-project/web/blob/main/lib/trento/software_updates/discovery.ex#L309This piece of code is potentially dangerous as it creates not existing atoms with data coming from external sources (it uses
String.to_atombehind the scenes).It is true that this information is coming from SUMA and we know the sent values so it should be safe.
But knowing the values, having a embedded schema makes sense as it gives some more control and we don't need to relay in jsonb plain maps.
I have tested a transition from old stored code to the new and works fine. (
embeds_manytakes care of using already existing empty list and nil values, and always return a list).As a bonus, we fix the openapi schema error, as the
to_package_idfield returns an integer and we define it as string. Now this is fixed.We most probably had this issue as SUMA docs themselves define this as string, even though they send an integer and they confirmed that this is a docs bug:
https://documentation.suse.com/suma/4.3/api/suse-manager/api/system.html#apidoc-system-listLatestUpgradablePackages-loggedInUser-sid
How was this tested?
UT and manual testing to test the regression