Code review comment for ~lamoura/ubuntu/+source/ubuntu-advantage-tools:upload-29-mantic

Hi Robie, thanks for the review.

Let me address all of the comments here:

> b33fb5e3 lock: alert the user about corrupted lock

This commit is just trying to fix a rebase issue we had in the past. It just deletes duplicated lines on the messages module. It is not ideal, but I think this can be ignored for the release.

> 31572946 refactor: implementation-agnostic interface to readurl

Yes, since the libraries we support now have those differences between how they raise the exception, we have tried to follow a pattern similar to what request does:
https://requests.readthedocs.io/en/latest/user/quickstart/#response-status-codes
But we understand that we need to be careful on the review regarding new code that perform any type of requests

> 296edca3 chore: rename repo GPG keys from -advantage to -pro
> 83808325 postinst: use for loop to rename gpg keys
> Why are we renaming these keys at all?

we plan to rename the package before 24.04 and we are already addressing some parts of the code earlier, to make this change more incremental.

> And there are further complications: these are in /etc, so could be modified or removed by users, so we have to consider that type of case - at least to review to ensure that the maintainer scripts cannot explode. Further, even though mv_conffile is not appropriate in this case, it demonstrates the error rollbacks that are not implemented here, so a failure somewhere else in the postinst could lead to a user's system ending up in a broken state due to this part not being rolled back correctly, and that requires further thought, review and possible implementation. Is this worth the regression risk and review effort?

Regarding user modifications, I believe we shouldn't worry. Those files are only created once the service is enabled and the key there isn unique for each service. Therefore, modifications are definitely unsupported.

Regarding failures, the only impact is that we will end up with a key that was not renamed. In that situation, the service will still work as expected and the only side-effect is that during disable, we will
not remove the old key. But I will of course create a test script that demonstrate that

> 7c2f481b http: use pycurl for tls-in-tls requests

Great catch, we need to better alert the user about ca-certificates. I will add a commit addressing this.

> fead7d19 contract: support requiredPackages directive

[Needs Information] Under what circumstances are these directives provided? Just when a service is being enabled, or is it wider than this?

We will only read and act upon this directive when a service is enabled. Also this directive is currently only present for the Landscape service

[Needs Fixing]

Yes, I will update the test and update the comment on the pin file too