Code review comment for ~chad.smith/ubuntu/+source/ubuntu-advantage-tools:pkg-upload-24.4

> I've had a chance to study the code and think I understand it. I have a
> couple questions to clarify before approving. I also have several code
> suggestions but these are just code style so not required or expected for this
> upload, and provided just for future refactoring ideas if you wish.

>
> ### Questions ###
>
> In cc.py and cis.py the packages data member is dropped, allowing these
> classes to rely on the base class' packages() property, which provides a list
> of packages from the configuration.

Correct base class implementation should work for most repo-based subclasses. FIPS* have an unique behavior in that -hmac packages need to additionally be installed only if the corresponding base package is already installed on the system which is why there is a fips-override of that behavior (so it doesn't automatically install all packages listed in additionalPackages directive).

> fips.py does something similar but
> overrides the base packages() prop with its own, dropping the fips_packages
> dict. Two questions:
>
> a. The old code specified two packages "ubuntu-commoncriteria" and "ubuntu-
> cisbenchmark-16.04". My assumption is that these are going to be loaded out
> of a config file going forward? Guessing this is the "contract server"
> mentioned in bug 1173?

Correct, here is an excerpt of that "contract" and cis service itself is a beta service that isn't even supported/installable at the moment, so no risk there at the moment.

               "type" : "cc-eal",
               "directives" : {
                  "additionalPackages" : [
                     "ubuntu-commoncriteria"
                  ],
                  "aptURL" : "https://esm.ubuntu.com/cc",
                  "aptKey" : "9F912DADD99EE1CC6BFFFF243A186E733F491C46",
                  "suites" : [
                     "xenial"
                  ]
               },
...

>
> b. Is there a risk if a user already had a config file that did not have
> these values, of things breaking? Or do we 100% control the contract server
> and already have the required data in place?
>

Any connected machine will already have the /var/lib/ubuntu-advantage/private/machine-token.json present with these additionalPackages directives already present. This has directive been in place for 1.5 years I believe. That said, Groovy doesn't offer/install said services, so this is really a noop for groovy. And yes we (online services team) 100% control the contract server details and have had this in place for a long time. ua-tools pkg just didn't use it in release-24 branch yet.

>
>
> ### Code Suggestions (Optional) ###
>
> 1. I notice in the test cases, test_cc.py uses "ubuntu-commoncriteria" as its
> sample data, but test_cis.py uses "pkg1". Would it be more convenient to use
> "pkg1" in both cases, or alternatively use "ubuntu-cisbenchmark-16.04" in
> test_cis.py

+1 we can take this to master branch.

>
> 2. repo.py's packages() prop adds logic to lookup
> "entitlement"->"directives"->"additionalPackages" from the configuration and
> then return it, or an empty list if not found. It strikes me this might be a
> situation where the code would be clearer to just wrap in a try block:
>
> try:
> return self.cfg.entitlements[self.name]['entitlement']['directives']['
> additionalPackages']
> except:
> return []

good thought, I'll put up a PR with this.

>
> 3. The packages() prop in repo.py could use a bit more explanation in its
> code doc. E.g.:
>
> def packages(self) -> "List[str]":
> """
> Packages to install on enablement.
>
> Returns the list of packages specified in the additionalPackages
> directive, or an empty list if none were configured.
> """
>

+1 will do

> 4. fips.py's packages() prop has an interesting use of groupby (I had to look
> that up, wasn't familiar with it - nice). But as the logic here is a bit more
> sophisticated it would be worthwhile to give this packages() property its own
> code docs. For example, maybe something like:
>
> @property
> def packages(self) -> "List[str]":
> """
> Packages to install on enablement.
>
> Returns only packages from additionalPackages that are
> either already installed or required for fips.
> """
>

+1 here too

> 5. Also in fips.py's packages() prop, the list assembler at the end could be
> shortened by combining the if statements since they have the same body:
>
> for pkg_name, pkg_list in pkg_groups:
> if pkg_name in installed_packages + self.fips_required_packages:
> packages += pkg_list

+1