Fix unhandled apt_configure case (SC-526)

[holmanb]

Proposed Commit Message

Fix schema and fix unhandled apt_configure case.

Don't throw an exception when mirror arch is unspecified.

Test Steps

Use the following config.

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[TheRealFalcon]

If mirror_cfg_elem = {'arches', None}, we still have a problem. I haven't checked so this might not even be possible, but just in general for these sorts of operations I prefer something more like

arches = mirror_cfg_elem.get("arches") or []

[holmanb]

Good catch, that looks like a safer pattern to use.

[holmanb]

A second look at this suggests that setting arches to an empty iterable might not be the right/only solution to this problem.

From the docs:

Each config is a dictionary which must have an entry for arches, specifying which architectures that config entry is for.

For a security mirror config:

  security:
    - uri: http://security.ubuntu.com/ubuntu

Consider the following resulting log:

2021-10-12 22:56:19,342 - cc_apt_configure.py[DEBUG]: got primary mirror: http://us.archive.ubuntu.com/ubuntu
2021-10-12 22:56:19,342 - cc_apt_configure.py[DEBUG]: got security mirror: None
2021-10-12 22:56:19,343 - cc_apt_configure.py[DEBUG]: Apt Mirror info: {'PRIMARY': 'http://us.archive.ubuntu.com/ubuntu', 'SECURITY': 'http://us.archive.ubuntu.com/ubuntu', 'MIRROR': 'http://us.archive.ubuntu.com/ubuntu'}

Note that the security mirror falls back to the primary mirror.

It looks like the schema attempts to make arches 'required', however I didn't see any warnings about arches in the logs.

mirror_property = {
    'type': 'array',
    'item': {
        'type': 'object',
        'additionalProperties': False,
        'required': ['arches'],
        'properties': {

Considering this config doesn't follow the rules "must have an entry for arches", I would expect a warning log to be dropped at a minimum, rather than failing over to the primary mirror without any mention that the config is invalid.

I think probably the right solution is in addition to arches = mirror_cfg_elem.get("arches") or [] we also "fix" the schema to log the config violation properly. Thoughts?

[TheRealFalcon]

Good catch about the schema. Yes, we should fail schema validation on that and emit a warning. Though I still think we should handle the empty case.

[holmanb]

Good catch about the schema. Yes, we should fail schema validation on that and emit a warning. Though I still think we should handle the empty case.

Agreed

[holmanb]

I'm still working on this. I noticed another schema issue with this module:
From this example

    my-repo3.list:
      # this would have the same end effect as 'ppa:curtin-dev/test-archive'
      source: "deb http://ppa.launchpad.net/curtin-dev/test-archive/ubuntu xenial main"
      keyid: F430BBA5 # GPG key ID published on the key server
      filename: curtin-dev-ppa.list

Note that the filename key is not documented in the apt_configure module docs, but the key is referenced and used in the implementation (it causes a duplicate *.list file to be dropped in /etc/apt/sources.list.d iirc). This should be documented in readthedocs and the schema

[holmanb]

The failing test pulls in the example from readthedocs and runs it. The test is fixed in #1068, so I can either pull that fix into this one, or wait until that one is merged.

[TheRealFalcon]

Easiest to just wait for #1068 and then rebase I think.

[holmanb]

I had to tweak some tests after rebasing. I think it's ready for review now.

[TheRealFalcon]

Thanks!