diff -Nru cloud-init-20.2-30-g8bcf1c06/cloudinit/config/cc_snap.py cloud-init-20.2-38-g8377897b/cloudinit/config/cc_snap.py --- cloud-init-20.2-30-g8bcf1c06/cloudinit/config/cc_snap.py 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/cloudinit/config/cc_snap.py 2020-05-27 19:11:58.000000000 +0000 @@ -93,6 +93,13 @@ - ['snap', 'install', 'vlc'] - snap install vlc - 'snap install vlc' + """), dedent("""\ + # You can use a list of assertions + snap: + assertions: + - signed_assertion_blob_here + - | + signed_assertion_blob_here """)], 'frequency': PER_INSTANCE, 'type': 'object', @@ -106,7 +113,8 @@ 'additionalItems': False, # Reject items non-string 'minItems': 1, 'minProperties': 1, - 'uniqueItems': True + 'uniqueItems': True, + 'additionalProperties': {'type': 'string'}, }, 'commands': { 'type': ['object', 'array'], # Array of strings or dict @@ -136,10 +144,6 @@ } } -# TODO schema for 'assertions' and 'commands' are too permissive at the moment. -# Once python-jsonschema supports schema draft 6 add support for arbitrary -# object keys with 'patternProperties' constraint to validate string values. - __doc__ = get_schema_doc(schema) # Supplement python help() SNAP_CMD = "snap" diff -Nru cloud-init-20.2-30-g8bcf1c06/cloudinit/config/tests/test_resolv_conf.py cloud-init-20.2-38-g8377897b/cloudinit/config/tests/test_resolv_conf.py --- cloud-init-20.2-30-g8bcf1c06/cloudinit/config/tests/test_resolv_conf.py 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/cloudinit/config/tests/test_resolv_conf.py 2020-05-27 19:11:58.000000000 +0000 @@ -35,8 +35,8 @@ # Patch in templater so we can assert on the actual generated content @mock.patch("cloudinit.templater.util.write_file") # Parameterise with the value to be passed to generate_resolv_conf as the - # `params` parameter, and a list of the expected lines after the header as - # `extra_lines`. + # params parameter, and the expected line after the header as + # expected_extra_line. @pytest.mark.parametrize( "params,expected_extra_line", [ diff -Nru cloud-init-20.2-30-g8bcf1c06/cloudinit/config/tests/test_snap.py cloud-init-20.2-38-g8377897b/cloudinit/config/tests/test_snap.py --- cloud-init-20.2-30-g8bcf1c06/cloudinit/config/tests/test_snap.py 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/cloudinit/config/tests/test_snap.py 2020-05-27 19:11:58.000000000 +0000 @@ -342,6 +342,20 @@ " of the given schemas\n", self.logs.getvalue()) + @mock.patch('cloudinit.config.cc_snap.run_commands') + def test_schema_when_assertions_values_are_invalid_type(self, _): + """Warnings when snap:assertions values are invalid type (e.g. int)""" + validate_cloudconfig_schema( + {'snap': {'assertions': [123]}}, schema) + validate_cloudconfig_schema( + {'snap': {'assertions': {'01': 123}}}, schema) + self.assertEqual( + "WARNING: Invalid config:\n" + "snap.assertions.0: 123 is not of type 'string'\n" + "WARNING: Invalid config:\n" + "snap.assertions.01: 123 is not of type 'string'\n", + self.logs.getvalue()) + @mock.patch('cloudinit.config.cc_snap.add_assertions') def test_warn_schema_assertions_is_not_list_or_dict(self, _): """Warn when snap:assertions config is not a list or dict.""" diff -Nru cloud-init-20.2-30-g8bcf1c06/cloudinit/conftest.py cloud-init-20.2-38-g8377897b/cloudinit/conftest.py --- cloud-init-20.2-30-g8bcf1c06/cloudinit/conftest.py 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/cloudinit/conftest.py 2020-05-27 19:11:58.000000000 +0000 @@ -2,6 +2,8 @@ import pytest +from cloudinit import util + @pytest.yield_fixture(autouse=True) def disable_subp_usage(request): @@ -20,18 +22,51 @@ def test_whoami(self): util.subp(["whoami"]) + To instead allow util.subp usage for a specific command, you can set the + parameter passed to this fixture to that command: + + @pytest.mark.parametrize("disable_subp_usage", ["bash"], indirect=True) + def test_bash(self): + util.subp(["bash"]) + + To specify multiple commands, set the parameter to a list (note the + double-layered list: we specify a single parameter that is itself a list): + + @pytest.mark.parametrize( + "disable_subp_usage", ["bash", "whoami"], indirect=True) + def test_several_things(self): + util.subp(["bash"]) + util.subp(["whoami"]) + This fixture (roughly) mirrors the functionality of CiTestCase.allowed_subp. N.B. While autouse fixtures do affect non-pytest tests, CiTestCase's allowed_subp does take precedence (and we have TestDisableSubpUsageInTestSubclass to confirm that). - - TODO: - * Enable select subp usage (i.e. allowed_subp=[...]) """ should_disable = getattr(request, "param", True) if should_disable: + if not isinstance(should_disable, (list, str)): + def side_effect(args, *other_args, **kwargs): + raise AssertionError("Unexpectedly used util.subp") + else: + # Look this up before our patch is in place, so we have access to + # the real implementation in side_effect + subp = util.subp + + if isinstance(should_disable, str): + should_disable = [should_disable] + + def side_effect(args, *other_args, **kwargs): + cmd = args[0] + if cmd not in should_disable: + raise AssertionError( + "Unexpectedly used util.subp to call {} (allowed:" + " {})".format(cmd, ",".join(should_disable)) + ) + return subp(args, *other_args, **kwargs) + with mock.patch('cloudinit.util.subp', autospec=True) as m_subp: - m_subp.side_effect = AssertionError("Unexpectedly used util.subp") + m_subp.side_effect = side_effect yield else: yield diff -Nru cloud-init-20.2-30-g8bcf1c06/cloudinit/tests/test_conftest.py cloud-init-20.2-38-g8377897b/cloudinit/tests/test_conftest.py --- cloud-init-20.2-30-g8bcf1c06/cloudinit/tests/test_conftest.py 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/cloudinit/tests/test_conftest.py 2020-05-27 19:11:58.000000000 +0000 @@ -21,6 +21,25 @@ def test_subp_usage_can_be_reenabled(self): util.subp(['whoami']) + @pytest.mark.parametrize( + 'disable_subp_usage', [['whoami'], 'whoami'], indirect=True) + def test_subp_usage_can_be_conditionally_reenabled(self): + # The two parameters test each potential invocation with a single + # argument + with pytest.raises(AssertionError) as excinfo: + util.subp(["some", "args"]) + assert "allowed: whoami" in str(excinfo.value) + util.subp(['whoami']) + + @pytest.mark.parametrize( + 'disable_subp_usage', [['whoami', 'bash']], indirect=True) + def test_subp_usage_can_be_conditionally_reenabled_for_multiple_cmds(self): + with pytest.raises(AssertionError) as excinfo: + util.subp(["some", "args"]) + assert "allowed: whoami,bash" in str(excinfo.value) + util.subp(['bash', '-c', 'true']) + util.subp(['whoami']) + class TestDisableSubpUsageInTestSubclass(CiTestCase): """Test that disable_subp_usage doesn't impact CiTestCase's subp logic.""" diff -Nru cloud-init-20.2-30-g8bcf1c06/config/cloud.cfg.tmpl cloud-init-20.2-38-g8377897b/config/cloud.cfg.tmpl --- cloud-init-20.2-30-g8bcf1c06/config/cloud.cfg.tmpl 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/config/cloud.cfg.tmpl 2020-05-27 19:11:58.000000000 +0000 @@ -125,11 +125,9 @@ {% if variant in ["ubuntu", "unknown"] %} - ubuntu-drivers {% endif %} -{% if variant not in ["freebsd", "netbsd"] %} - puppet - chef - mcollective -{% endif %} - salt-minion - rightscale_userdata - scripts-vendor diff -Nru cloud-init-20.2-30-g8bcf1c06/debian/changelog cloud-init-20.2-38-g8377897b/debian/changelog --- cloud-init-20.2-30-g8bcf1c06/debian/changelog 2020-05-18 22:24:16.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/debian/changelog 2020-05-27 20:36:46.000000000 +0000 @@ -1,3 +1,19 @@ +cloud-init (20.2-38-g8377897b-0ubuntu1) groovy; urgency=medium + + * New upstream snapshot. + - enable Puppet, Chef mcollective in default config (#385) + [Mina Galić (deprecated: Igor Galić)] (LP: #1880279) + - HACKING.rst: introduce .net -> Networking refactor section (#384) + - Travis: do not install python3-contextlib2 (dropped dependency) (#388) + [Paride Legovini] + - HACKING: mention that .github-cla-signers is alpha-sorted (#380) + - Add bipinbachhao as contributor (#379) [Bipin Bachhao] + - cc_snap: validate that assertions property values are strings (#370) + - conftest: implement partial disable_subp_usage (#371) + - test_resolv_conf: refresh stale comment (#374) + + -- Chad Smith Wed, 27 May 2020 14:36:46 -0600 + cloud-init (20.2-30-g8bcf1c06-0ubuntu1) groovy; urgency=medium * d/control: drop pyflakes as Build-Depends diff -Nru cloud-init-20.2-30-g8bcf1c06/HACKING.rst cloud-init-20.2-38-g8377897b/HACKING.rst --- cloud-init-20.2-30-g8bcf1c06/HACKING.rst 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/HACKING.rst 2020-05-27 19:11:58.000000000 +0000 @@ -67,6 +67,8 @@ * See `PR #344`_ and `PR #345`_ for examples of what this pull request should look like. + * Note that ``.github-cla-signers`` is sorted alphabetically. + * (If you already have a change that you want to submit, you can also include the change to ``tools/.github-cla-signers`` in that pull request, there is no need for two separate PRs.) @@ -321,3 +323,115 @@ py.test-3 --fixtures -q | grep "^[^ ]" | grep -v no | sed 's/.*/* ``\0``/' in a xenial lxd container with python3-pytest installed. + +Ongoing Refactors +================= + +This captures ongoing refactoring projects in the codebase. This is +intended as documentation for developers involved in the refactoring, +but also for other developers who may interact with the code being +refactored in the meantime. + +``cloudinit.net`` -> ``cloudinit.distros.Networking`` Hierarchy +--------------------------------------------------------------- + +``cloudinit.net`` was imported from the curtin codebase as a chunk, and +then modified enough that it integrated with the rest of the cloud-init +codebase. Over the ~4 years since, the fact that it is not fully +integrated into the ``Distro`` hierarchy has caused several issues. + +The common pattern of these problems is that the commands used for +networking are different across distributions and operating systems. +This has lead to ``cloudinit.net`` developing its own "distro +determination" logic: `get_interfaces_by_mac`_ is probably the clearest +example of this. Currently, these differences are primarily split +along Linux/BSD lines. However, it would be short-sighted to only +refactor in a way that captures this difference: we can anticipate that +differences will develop between Linux-based distros in future, or +there may already be differences in tooling that we currently +work around in less obvious ways. + +The high-level plan is to introduce a hierarchy of networking classes +in ``cloudinit.distros``, which each ``Distro`` subclass will +reference. These will capture the differences between networking on +our various distros, while still allowing easy reuse of code between +distros that share functionality (e.g. most of the Linux networking +behaviour). Callers will call ``distro.net.func`` instead of +``cloudinit.net.func``, which will necessitate access to an +instantiated ``Distro`` object. + +An implementation note: there may be external consumers of the +``cloudinit.net`` module. We don't consider this a public API, so we +will be removing it as part of this refactor. However, we will ensure +that the new API is complete from its introduction, so that any such +consumers can move over to it wholesale. (Note, however, that this new +API is still not considered public or stable, and may not replicate the +existing API exactly.) + +In more detail: + +* The root of this hierarchy will be the + ``cloudinit.distros.Networking`` class. This class will have + a corresponding method for every ``cloudinit.net`` function that we + identify to be involved in refactoring. Initially, these methods' + implementations will simply call the corresponding ``cloudinit.net`` + function. (This gives us the complete API from day one, for existing + consumers.) +* As the biggest differentiator in behaviour, the next layer of the + hierarchy will be two subclasses: ``LinuxNetworking`` and + ``BSDNetworking``. These will be introduced in the initial PR. +* When a difference in behaviour for a particular distro is identified, + a new ``Networking`` subclass will be created. This new class should + generally subclass either ``LinuxNetworking`` or ``BSDNetworking``. +* To be clear: ``Networking`` subclasses will only be created when + needed, we will not create a full hierarchy of per-``Distro`` + subclasses up-front. +* Each ``Distro`` class will have a class variable + (``cls.networking_cls``) which points at the appropriate + networking class (initially this will be either ``LinuxNetworking`` + or ``BSDNetworking``). +* When ``Distro`` classes are instantiated, they will instantiate + ``cls.networking_cls`` and store the instance at ``self.net``. (This + will be implemented in ``cloudinit.distros.Distro.__init__``.) +* A helper function will be added which will determine the appropriate + ``Distro`` subclass for the current system, instantiate it and return + its ``net`` attribute. (This is the entry point for existing + consumers to migrate to.) +* Callers of refactored functions will change from calling + ``cloudinit.net.some_func`` to ``distro.net.some_func``, where + ``distro`` is an instance of the appropriate ``Distro`` class for + this system. (This will require making such an instance available to + callers, which will constitute a large part of the work in this + project.) + +After the initial structure is in place, the work in this refactor will +consist of replacing the ``cloudinit.net.some_func`` call in each +``cloudinit.distros.Networking`` method with the actual implementation. +This can be done incrementally, one function at a time: + +* pick an unmigrated ``cloudinit.distros.Networking`` method +* refactor all of its callers to call the ``distro.net`` method on + ``Distro`` instead of the ``cloudinit.net`` function. (This is likely + to be the most time-consuming step, as it may require plumbing + ``Distro`` objects through to places that previously have not + consumed them.) +* refactor its implementation from ``cloudinit.net`` into the + ``Networking`` hierarchy (e.g. if it has an if/else on BSD, this is + the time to put the implementations in their respective subclasses) +* ensure that the new implementation has unit tests (either by moving + existing tests, or by writing new ones) +* finally, remove it (and any other now-unused functions) from + cloudinit.net (to avoid having two parallel implementations) + +References +~~~~~~~~~~ + +* `Mina Galić's email the the cloud-init ML in 2018`_ (plus its thread) +* `Mina Galić's email to the cloud-init ML in 2019`_ (plus its thread) +* `PR #363`_, the discussion which prompted finally starting this + refactor (and where a lot of the above details were hashed out) + +.. _get_interfaces_by_mac: https://github.com/canonical/cloud-init/blob/961239749106daead88da483e7319e9268c67cde/cloudinit/net/__init__.py#L810-L818 +.. _Mina Galić's email the the cloud-init ML in 2018: https://lists.launchpad.net/cloud-init/msg00185.html +.. _Mina Galić's email to the cloud-init ML in 2019: https://lists.launchpad.net/cloud-init/msg00237.html +.. _PR #363: https://github.com/canonical/cloud-init/pull/363 diff -Nru cloud-init-20.2-30-g8bcf1c06/tools/.github-cla-signers cloud-init-20.2-38-g8377897b/tools/.github-cla-signers --- cloud-init-20.2-30-g8bcf1c06/tools/.github-cla-signers 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/tools/.github-cla-signers 2020-05-27 19:11:58.000000000 +0000 @@ -1,4 +1,5 @@ beezly +bipinbachhao dhensby lucasmoura nishigori diff -Nru cloud-init-20.2-30-g8bcf1c06/.travis.yml cloud-init-20.2-38-g8377897b/.travis.yml --- cloud-init-20.2-30-g8bcf1c06/.travis.yml 2020-05-18 17:30:27.000000000 +0000 +++ cloud-init-20.2-38-g8377897b/.travis.yml 2020-05-27 19:11:58.000000000 +0000 @@ -50,7 +50,7 @@ - sudo apt-get build-dep -y cloud-init - sudo apt-get install -y --install-recommends sbuild ubuntu-dev-tools fakeroot tox # These are build deps but not pulled in by the build-dep call above - - sudo apt-get install -y --install-recommends dh-systemd python3-coverage python3-contextlib2 python3-pytest python3-pytest-cov + - sudo apt-get install -y --install-recommends dh-systemd python3-coverage python3-pytest python3-pytest-cov - pip install . - pip install tox # bionic has lxd from deb installed, remove it first to ensure