Merge ~chad.smith/ubuntu/+source/ubuntu-advantage-tools:pkg-upload-24.4 into ubuntu/+source/ubuntu-advantage-tools:ubuntu/devel

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. 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?

  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?

### 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

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 []

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.
           """

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.
         """

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

tox passes for me, modulo the known-issue (#1062) which showed for xenial and bionic:

  py3: commands succeeded
  flake8: commands succeeded
  py3-xenial: commands succeeded
  py3-bionic: commands succeeded
  py3-eoan: commands succeeded
  flake8-trusty: commands succeeded
ERROR: flake8-xenial: commands failed
ERROR: flake8-bionic: commands failed
  flake8-eoan: commands succeeded
  mypy: commands succeeded
  black: commands succeeded

> 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 ...

I saw `tox -e behave` listed in README.md as an integration test, so tried running that too. I don't know if that is an applicable testsuite for acceptance testing, nor if it is currently expected to pass or not, but it failed for me:

~/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools$ tox -e behave
GLOB sdist-make: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/setup.py
behave create: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/behave
behave installdeps: -rrequirements.txt, -rtest-requirements.txt
behave inst: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/.tmp/package/1/ubuntu-advantage-tools-24.4.zip
behave installed: appdirs==1.4.3,attrs==20.1.0,behave==1.2.6,CacheControl==0.12.6,certifi==2019.11.28,chardet==3.0.4,colorama==0.4.3,contextlib2==0.6.0,coverage==5.2.1,distlib==0.3.0,distro==1.4.0,flake8==3.8.3,html5lib==1.0.1,idna==2.8,iniconfig==1.0.1,ipaddr==2.2.0,lockfile==0.12.2,mccabe==0.6.1,mock==4.0.2,more-itertools==8.5.0,msgpack==0.6.2,packaging==20.3,parse==1.17.0,parse-type==0.5.2,pep517==0.8.2,pluggy==0.13.1,progress==1.5,py==1.9.0,pycodestyle==2.6.0,pyflakes==2.2.0,PyHamcrest==2.0.2,pyparsing==2.4.6,pytest==6.0.1,pytest-cov==2.10.1,pytoml==0.1.21,PyYAML==5.3.1,requests==2.22.0,retrying==1.3.3,six==1.14.0,toml==0.10.1,ubuntu-advantage-tools==24.4,urllib3==1.25.8,webencodings==0.5.1
behave run-test-pre: PYTHONHASHSEED='3371048301'
behave run-test: commands[0] | behave --verbose
Loading config defaults from "./tox.ini"
Using defaults:
          color True
  show_snippets True
   show_skipped True
        dry_run False
    show_source True
   show_timings True
 stdout_capture True
 stderr_capture True
    log_capture True
 logging_format %(levelname)s:%(name)s:%(message)s
  logging_level 20
  steps_catalog False
        summary True
          junit False
          stage None
       userdata {}
 default_format pretty
   default_tags
scenario_outline_annotation_schema {name} -- @{row.id} {examples.name}
more_formatters {}
Using default path "./features"
Trying base directory: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/features
Config options:
  image_clean = True
  destroy_instances = True
  contract_token = None
  reuse_image = None
Creating behave-image-build-1598996336365313
Starting behave-image-build-1598996336365313
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
HOOK-ERROR in before_all: Exception: System did not boot in 10s
  File "/home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/behave/lib/python3.8/site-packages/behave/runner.py", line 545, in run_hook
    self.hooks[name](context, *args)
  File "features/environment.py", line 111, in before_all
    create_trusty_uat_lxd_image(context)
  File "features/environment.py", line 167, in create_trusty_uat_lxd_image
    launch_lxd_container(context, "ubuntu:trusty", build_container_name)
  File "/home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/features/util.py", line 33, in la...

As mentioned in chat. behave isn't expected to pass on this branch as
.travis.yml marks that job:

  allow_failures:
    - python: 3.7
      env: TOXENV=behave
      script:

That said, when we release version 25.0 we expect the behave targets to pass

One other minor niggle, the indentation on the changelog entry is one space too many:

   * New bug-fix-only release 24.4:
      - uaclient.version bump to 24.4
      - fips: honor additionalPackage directive from contract for bionic
        (GH #1173)

I dedented locally for the upload but you may want to update your git tree too.

Otherwise, thanks for confirming my assumptions on the config data, so everything looks good for upload.

$ dput ubuntu ubuntu-advantage-tools_24.4_source.changes
Checking signature on .changes
gpg: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools_24.4_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools_24.4.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading ubuntu-advantage-tools_24.4.dsc: done.
  Uploading ubuntu-advantage-tools_24.4.tar.xz: done.
  Uploading ubuntu-advantage-tools_24.4_source.buildinfo: done.
  Uploading ubuntu-advantage-tools_24.4_source.changes: done.
Successfully uploaded packages.