> 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