feat: third party resource checksum

[jneo8]

Check upload resources sha256 hash and architecture support.

[dashmage]

Left a few comments after going through your code changes.

Let me know if there's any fault in my understanding of your changes :)

[jneo8]

Hi @dashmage, I update the PR to address your comments. Can I have your review again?

[chanchiwai-ray]

I left some inline comments about some typos.

Needs information: Do you think we should make use of the rest of the information in ToolVersionInfo? The validate_checksum function only uses sha256_checksum and supported_series data, maybe we should provide logging message about where to find the appropriate resource ?

[sudeephb]

Apart from the check sum -> checksum changes, I have a question on whether os_platform.py itself is necessary or an overkill. We could achieve the same functionality using distro.info() combined with a simple dictionary of ubuntu series mappings for the comparison of supported_series and the series of the machine being run on.

[dashmage]

@jneo8 I've resolved my conversations, thanks for checking them out. If you could fix those typos brought up in the other reviews, it's a +1 from me.

[jneo8]

Apart from the check sum -> checksum changes, I have a question on whether os_platform.py itself is necessary or an overkill. We could achieve the same functionality using distro.info() combined with a simple dictionary of ubuntu series mappings for the comparison of supported_series and the series of the machine being run on.

That is the implementation I try to avoid.

Try to use declarative way, using enum to define UbuntuSeries, is really helpful to make new comer understand the code easily, make them more easy to add more support versions and avoid the possible error. Also os_platform provide a flexible to add more distro relate functions in the future. Also it's a independent module only response to handle the os platform, which means it's more easy to test. Overall if we compare the way right now to multiple if-else syntax + distro dictionary, I don't see benefit from it.

[jneo8]

I left some inline comments about some typos.

Needs information: Do you think we should make use of the rest of the information in ToolVersionInfo? The validate_checksum function only uses sha256_checksum and supported_series data, maybe we should provide logging message about where to find the appropriate resource ?

Other information in ToolVersionInfo are version, link, and desc.
Logging about those information maybe a noise from my point of view.
Instead of logging, it will be nice if we can have those information in the document instead of logging, it's also the reason why I insist we should maintain our document in the repo before.

[chanchiwai-ray]

Other information in ToolVersionInfo are version, link, and desc. Logging about those information maybe a noise from my point of view. Instead of logging, it will be nice if we can have those information in the document instead of logging, it's also the reason why I insist we should maintain our document in the repo before.

Are those information really necessary in the code if it's not used anywhere? It sound like those are supplementary information that can be put in a doc / discourse post, and then in the code put a link to the doc / discourse post.

[sudeephb]

it's a independent module only response to handle the os platform, which means it's more easy to test. Overall if we compare the way right now to multiple if-else syntax + distro dictionary, I don't see benefit from it.

There will be no if-else statements. I agree with using enums for series information, you can disregard me mentioning a dict. However, can we declare the UbuntuSeries enum in checksum.py and replace this line with something like -
distro.info()['version'] in info.supported_series (with appropriate formatting/modifications, if necessary). My main point of confusion is do we really need the OSPlatform class and get_os_platform function when we can directly get the series information from a simple function call distro.info()?

[jneo8]

Other information in ToolVersionInfo are version, link, and desc. Logging about those information maybe a noise from my point of view. Instead of logging, it will be nice if we can have those information in the document instead of logging, it's also the reason why I insist we should maintain our document in the repo before.

Are those information really necessary in the code if it's not used anywhere? It sound like those are supplementary information that can be put in a doc / discourse post, and then in the code put a link to the doc / discourse post.

I will say yes otherwise you can't easily review the checksum & version information in the PR. Imagine you get a new PR which only include the checksum hash without those information and how many trouble you will get if you are the maintainer.

[jneo8]

I left some inline comments about some typos.
Needs information: Do you think we should make use of the rest of the information in ToolVersionInfo? The validate_checksum function only uses sha256_checksum and supported_series data, maybe we should provide logging message about where to find the appropriate resource ?

Other information in ToolVersionInfo are version, link, and desc. Logging about those information maybe a noise from my point of view. Instead of logging, it will be nice if we can have those information in the document instead of logging, it's also the reason why I insist we should maintain our document in the repo before.

There will be no if-else statements. I agree with using enums for series information, you can disregard me mentioning a dict. However, can we declare the UbuntuSeries enum in checksum.py and replace this line with something like - distro.info()['version'] in info.supported_series (with appropriate formatting/modifications, if necessary). My main point of confusion is do we really need the OSPlatform class and get_os_platform function when we can directly get the series information from a simple function call distro.info()?

We also use the platform package to get the information. I suppose you can't easily add those logic in your example.

Another reason I insist to put os platform as a single module also include I expected it will get more and more complex over time.

[dashmage]

I will say yes otherwise you can't easily review the checksum & version information in the PR. Imagine you get a new PR which only include the checksum hash without those information and how many trouble you will get if you are the maintainer.

Maybe the info would work as comments then? If it's not actively being used by the code, I don't think we should have those vars since it could be misleading.

And for the PR example, we can assume that ideally they should include the other supplementary info with the PR so that reviewers can verify.

[chanchiwai-ray]

I will say yes otherwise you can't easily review the checksum & version information in the PR. Imagine you get a new PR which only include the checksum hash without those information and how many trouble you will get if you are the maintainer.

Yeah, it will be convenient for the maintainer. But the difference is only whether you can see the version information on the same page v.s. you need to open another tab for the version information. It's not a blocker, I am okay with your current approach though.

I am trying to see if we can simplify the data structure that can improve the performance a bit. For example, without version information. We can simply have a map from series to list_of_supported_hashes, and we can avoid the for-loop. (Though, the loop is likely to be very small).

[sudeephb]

We also use the platform package to get the information. I suppose you can't easily add those logic in your example.

Yes, that is part of my point. If we drop the logic to get system info using platform package and just go with distro.info without checking whether it's an ubuntu system or not(because the charmhub page already mentions that the charm is supposed to be run on ubuntu), do we lose anything of value?

The reason I insist to put os platform as a single module also include I expected it will get more and more complex over time.

I do agree that it becomes more extendable as a module though.

[jneo8]

Maybe the info would work as comments then? If it's not actively being used by the code, I don't think we should have those vars since it could be misleading.

And for the PR example, we can assume that ideally they should include the other supplementary info with the PR so that reviewers can verify.

I try to force contributor to provide related information in the PR.

[jneo8]

Yes, that is part of my point. If we drop the logic to get system info using platform package and just go with distro.info without checking whether it's an ubuntu system or not(because the charmhub page already mentions that the charm is supposed to be run on ubuntu), do we lose anything of value?

That's the code try to support Ubuntu derivatives, I don't know why we don't support them.

[jneo8]

Yes, that is part of my point. If we drop the logic to get system info using platform package and just go with distro.info without checking whether it's an ubuntu system or not(because the charmhub page already mentions that the charm is supposed to be run on ubuntu), do we lose anything of value?

One more to add:

The practice is from here

@sudeephb
I would like to simple the questions:

1. Do we want to have os_platform module or get_os_platform function? -> I will answer yes even we don't use platform package to support ubuntu derivatives. I trend to make it modularize.
2. Do we want to support ubuntu derivatives? -> This can be discussed.

[Pjack]

There are good discussion. I try to split the questions one-by-one and provide my feedback in short.

1. Is it neccessary to put supplementary information in ToolVersionInfo (version, link, and desc)

• my choice will put it as a comment since it is not used in any logic
• Force the contributor to provide enough information is a good idea too.
• If this application will be installed on a device with only 128MB ram, I will say no. but it isn't. Therefore, I give +1 to current implementation

2. Do we need to support Ubuntu derivatives?

• I don't think we need. Even we support it, the value of "system" will be always "ubuntu". Please check my inline comment.

• Good to know Charmcraft support it. Therefore, we can build a charm on a ubuntu-like machine.

3. Overkill of OSPlatform ?

• maybe yes if we don't use machine information. But let's try to use it in this MR.

[jneo8]

Hi @Pjack, the newest commit already address your comments.

[Pjack]

only nitpick in the unit test. thanks!