Code review comment for ~renanrodrigo/ubuntu/+source/ubuntu-advantage-tools:upload-27.5-jammy

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Hi Renan,

As we discussed offline, I am leaving some notes here:

- The changes in the postrm script are missing in the changelog;

- The documentation (including help commands) changed to include a link for a page which does not exist (https://ubuntu.com/security/certifications/docs/usg);

- There are several new calls to `resource["name"]` and to resource.get("name"). This was somewhat confusing while I was reviewing the code, when I wondered if we should always expect that key to be present in that dict or not. The reason I am pointing this out is that we did need a hotfix in the past for a similar issue with a different dict key access;

- and finally, it is worth mentioning that there was a subtle behavior change when calls to `entitlements.entitlement_factory` replaced calls to `entitlements.ENTITLEMENT_CLASS_BY_NAME`. When a non-existent name would be passed to ENTITLEMENT_CLASS_BY_NAME, it would instantly raise a KeyError exception. Now, the code will either explode at a later, different point, or just present an odd behavior.

« Back to merge proposal