[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Commit Message changed to: make deb: Add dependency on devscripts for make deb and minor cleanup of packages/bddeb script. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requiremets.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:make-deb-cleanup into
cloud-init:master.
Commit message:
make deb: Add dependency on devscripts for make deb and minor cleanup of
packages/bddeb script.
Add a simple dependency check to "make deb" target for devscripts. Rework a bit
of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid
duplication of requirements already listed in (test-)?requiremets.txt. All
"standard" packages can be assumed to have either python3- or python- prefix if
not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside
write_debian_folder which is unneeded up in main.
Requested reviews:
cloud init development team (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076
make deb: Add dependency on devscripts for make deb and minor cleanup of
packages/bddeb script.
Add a simple dependency check to "make deb" target for devscripts. Rework a bit
of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid
duplication of requirements already listed in (test-)?requiremets.txt. All
"standard" packages can be assumed to have either python3- or python- prefix if
not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside
write_debian_folder which is unneeded up in main.
to test:
make deb # will print install instruction on a machine without devscripts
# check final product to make sure dependencies listed haven't changed for the
package
dpkg -I cloud-init_all.deb
--
Your team cloud init development team is requested to review the proposed merge
of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master.
diff --git a/Makefile b/Makefile
index 5940ed7..a76dac7 100644
--- a/Makefile
+++ b/Makefile
@@ -82,6 +82,10 @@ rpm:
./packages/brpm --distro $(distro)
deb:
+ @which debuild || \
+ { echo "Missing devscripts dependency. Install with:"; \
+ echo sudo apt-get install devscripts; exit 1; }
+
./packages/bddeb
.PHONY: test pyflakes pyflakes3 clean pep8 rpm deb yaml check_version
diff --git a/packages/bddeb b/packages/bddeb
index 79ac976..f415209 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -24,35 +24,17 @@ if "avoid-pep8-E402-import-not-top-of-file":
from cloudinit import templater
from cloudinit import util
-# Package names that will showup in requires to what we can actually
-# use in our debian 'control' file, this is a translation of the 'requires'
-# file pypi package name to a debian/ubuntu package name.
-STD_NAMED_PACKAGES = [
-'configobj',
-'coverage',
-'jinja2',
-'jsonpatch',
-'oauthlib',
-'prettytable',
-'requests',
-'six',
-'httpretty',
-'mock',
-'nose',
-'setuptools',
-'flake8',
-'hacking',
-'unittest2',
-]
+# Package names that will showup in requires which have unique package names.
+# Format is '': {'': , ...}.
NONSTD_NAMED_PACKAGES = {
-'argparse': ('python-argparse', None),
-'contextlib2': ('python-contextlib2', None),
-'cheetah': ('python-cheetah', None),
-'pyserial': ('python-serial', 'python3-serial'),
-'pyyaml': ('python-yaml', 'python3-yaml'),
-'six': ('python-six', 'python3-six'),
-'pep8': ('pep8', 'python3-pep8'),
-'pyflakes': ('pyflakes', 'pyflakes'),
+'argparse': {'2': 'python-argparse', '3': None},
+'contextlib2': {'2': 'python-contextlib2', '3': None},
+'cheetah': {'2': 'python-cheetah', '3': None},
+'pyserial': {'2': 'python-serial', '3': 'python3-serial'},
+'pyyaml': {'2': 'python-yaml', '3': 'python3-yaml'},
+'six': {'2': 'python-six', '3': 'python3-six'},
+'pep8': {'2': 'pep8', '3': 'python3-pep8'},
+'pyflakes': {'2': 'pyflakes', '3': 'pyflakes'},
}
DEBUILD_ARGS = ["-S", "-d"]
@@ -68,8 +50,17 @@ def run_helper(helper, args=None, strip=True):
return stdout
-def write_debian_folder(root, templ_data, pkgmap, pyver="3",
-append_requires=[]):
+def write_debian_folder(root, templ_data, is_python2, cloud_util
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Commit Message changed to: make deb: Add dependency on devscripts for make deb and minor cleanup of packages/bddeb script. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requiremets.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. LP:1685935 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Commit Message changed to: make deb: Add dependency on devscripts for make deb and minor cleanup of packages/bddeb script. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requiremets.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. LP: #1685935 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Description changed to: make deb: Add dependency on devscripts for make deb and minor cleanup of packages/bddeb script. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requiremets.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. to test: make deb # will print install instruction on a machine without devscripts # check final product to make sure dependencies listed haven't changed for the package dpkg -I cloud-init_all.deb Note: I didn't address reading build-deps directly from the debian/config.in template as the error message you receive about build-deps seemed fairly straight forward if some build-deps are missing. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cyaml-loading into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:cyaml-loading into cloud-init:master. Requested reviews: cloud init development team (cloud-init-dev) For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323088 Use CSafeLoader C-bindings instead of python for a faster yaml.load when processing yaml files. For faster yaml processing import CSafeLoader in cloudinit/safeyaml.py during yaml loads. Also adds a comment to requirements.txt about the libyaml-dev packages required for unit tests to properly build. Previous test deployments may need to run tox -r to rebuild the virtual env cache if necessary libyaml-dev python-dev packages were absent. LP: # 1685939 To test: tox -r tox -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:cyaml-loading into cloud-init:master. diff --git a/cloudinit/safeyaml.py b/cloudinit/safeyaml.py index 7bcf9dd..4dd9fb4 100644 --- a/cloudinit/safeyaml.py +++ b/cloudinit/safeyaml.py @@ -4,10 +4,12 @@ # # This file is part of cloud-init. See LICENSE file for license information. -import yaml +from yaml import ( +CSafeLoader, +load as yaml_load) -class _CustomSafeLoader(yaml.SafeLoader): +class _CustomSafeLoader(CSafeLoader): def construct_python_unicode(self, node): return self.construct_scalar(node) @@ -18,6 +20,6 @@ _CustomSafeLoader.add_constructor( def load(blob): -return(yaml.load(blob, Loader=_CustomSafeLoader)) +return(yaml_load(blob, Loader=_CustomSafeLoader)) # vi: ts=4 expandtab diff --git a/requirements.txt b/requirements.txt index 0c4951f..157592a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -24,10 +24,14 @@ oauthlib # section)... configobj>=5.0.2 -# All new style configurations are in the yaml format +# cloudinit/safeyaml.py depends on libyaml c-bindings which are built by tox +# when libyaml-(dev|devel), libpython2.7-(dev|devel) and libpython3-(dev|devel) +# headers packages are installed. If tox fails importing CSafeLoader, we need +# these dev packages. +# All new style configurations are in the yaml format. pyyaml -# The new main entrypoint uses argparse instead of optparse +# The new main entrypoint uses argparse instead of optparse argparse # Requests handles ssl correctly! ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cyaml-loading into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cyaml-loading into cloud-init:master has been updated. Status: Needs review => Work in progress For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323088 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:cyaml-loading into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Commit Message changed to: make deb: Add devscripts dependency for make deb. Cleanup packages/bddeb. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requiremets.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. LP: #1685935 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master
Diff comments:
> diff --git a/cloudinit/config/cc_disk_setup.py
> b/cloudinit/config/cc_disk_setup.py
> index f49386e..32d51e8 100644
> --- a/cloudinit/config/cc_disk_setup.py
> +++ b/cloudinit/config/cc_disk_setup.py
> @@ -936,14 +938,14 @@ def mkfs(fs_cfg):
> if overwrite or device_type(device) == "disk":
> fs_cmd.append(lookup_force_flag(fs_type))
>
> -# Add the extends FS options
> -if fs_opts:
> -fs_cmd.extend(fs_opts)
> +# Add the extends FS options
I haven't glanced at this scope change here with the new indents. trying to
think about the implications of not handling this in other cases.
> +if fs_opts:
> +fs_cmd.extend(fs_opts)
>
> LOG.debug("Creating file system %s on %s", label, device)
> -LOG.debug(" Using cmd: %s", " ".join(fs_cmd))
> +LOG.debug(" Using cmd: %s", fs_cmd if shell else " ".join(fs_cmd))
> try:
> -util.subp(fs_cmd)
> +util.subp(fs_cmd, shell=shell)
> except Exception as e:
> raise Exception("Failed to exec of '%s':\n%s" % (fs_cmd, e))
>
--
https://code.launchpad.net/~paul-meyer/cloud-init/+git/cloud-init/+merge/323532
Your team cloud init development team is requested to review the proposed merge
of ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master
Thanks for the bug and fix here paul.
Diff comments:
> diff --git a/cloudinit/config/cc_disk_setup.py
> b/cloudinit/config/cc_disk_setup.py
> index f49386e..5aeef4b 100644
> --- a/cloudinit/config/cc_disk_setup.py
> +++ b/cloudinit/config/cc_disk_setup.py
> @@ -910,12 +910,21 @@ def mkfs(fs_cfg):
> "must be set.", label)
>
> # Create the commands
> +shell = False
> if fs_cmd:
> fs_cmd = fs_cfg['cmd'] % {
> 'label': label,
> 'filesystem': fs_type,
> 'device': device,
> }
> +shell = True
> +
> +if overwrite:
> +LOG.warning('fs_setup option "overwrite" ignored because "cmd" '
> +
> +'was specified')
> +if fs_opts:
> +LOG.warning('fs_setup option "extra_opts" ignored because "cmd"
> ' +
Thanks for these, the more logging the better the user understanding during
failures.
Any objections to logging the actual command specified so the user can track
which list item in fs_setup is problematic? (You'd have to tweak your unit
tests a bit)
if overwrite:
LOG.warning(
"fs_setup:overwrite ignored because cmd was "
"specified: {}".format(fs_cmd))
if fs_opts:
LOG.warning(
"fs_setup:extra_opts ignored because cmd was "
"specified: {}".format(fs_cmd))
> +'was specified')
> else:
> # Find the mkfs command
> mkfs_cmd = util.which("mkfs.%s" % fs_type)
> @@ -936,14 +945,14 @@ def mkfs(fs_cfg):
> if overwrite or device_type(device) == "disk":
> fs_cmd.append(lookup_force_flag(fs_type))
>
> -# Add the extends FS options
> -if fs_opts:
> -fs_cmd.extend(fs_opts)
> +# Add the extends FS options
> +if fs_opts:
Thanks for clarifying in your earlier comment, yes you are right in your
assessment, extra_opts and cmd can't be used together. I'll put up a
documentation MP on this shortly to describe it once I chat with the team about
use-cases for extra_opts.
> +fs_cmd.extend(fs_opts)
>
> LOG.debug("Creating file system %s on %s", label, device)
> -LOG.debug(" Using cmd: %s", " ".join(fs_cmd))
> +LOG.debug(" Using cmd: %s", str(fs_cmd))
> try:
> -util.subp(fs_cmd)
> +util.subp(fs_cmd, shell=shell)
> except Exception as e:
> raise Exception("Failed to exec of '%s':\n%s" % (fs_cmd, e))
>
> diff --git a/tests/unittests/test_handler/test_handler_disk_setup.py
> b/tests/unittests/test_handler/test_handler_disk_setup.py
> index 7ff3922..36e53c7 100644
> --- a/tests/unittests/test_handler/test_handler_disk_setup.py
> +++ b/tests/unittests/test_handler/test_handler_disk_setup.py
> @@ -147,4 +151,49 @@ class TestUpdateFsSetupDevices(TestCase):
> 'filesystem': 'xfs'
> }, fs_setup)
>
> +
> +@mock.patch('cloudinit.config.cc_disk_setup.find_device_node',
> +return_value=('/dev/xdb1', False))
> +@mock.patch('cloudinit.config.cc_disk_setup.device_type', return_value=None)
> +@mock.patch('cloudinit.config.cc_disk_setup.util.subp', return_value=('',
> ''))
> +class TestMkfsCommandHandling(TestCase):
> +
> +def test_with_cmd(self, subp, *args):
Can we add a oneliner docstring about the intent of these tests:
def test_cmd_warns_about_overwrite_and_extra_opts(...)
"""mkfs honors cmd and logs warnings when extra_opts or overwrite are
provided."""
> +with self.assertLogs(
> +'cloudinit.config.cc_disk_setup') as logs:
> +cc_disk_setup.mkfs({
> +'cmd': 'mkfs -t %(filesystem)s -L %(label)s %(device)s',
> +'filesystem': 'ext4',
> +'device': '/dev/xdb1',
> +'label': 'with_cmd',
> +'extra_opts': ['should', 'generate', 'warning'],
> +'overwrite': 'should generate warning too'
> +})
> +
> +self.assertIn(
> +'WARNING:cloudinit.config.cc_disk_setup:' +
> +'fs_setup option "extra_opts" ignored because "cmd" was
> specified',
> +logs.output)
> +self.assertIn(
> +'WARNING:cloudinit.config.cc_disk_setup:' +
> +'fs_setup option "overwrite" ignored because "cmd" was
> specified',
> +logs.output)
> +
> +subp.assert_called_once_with(
> +'mkfs -t ext4 -L with_cmd /dev/xdb1', shell=True)
> +
> +def test_without_cmd(self, subp, *args):
test_overwrite_and_extra_opts_without_cmd(...)
"""mkfs observes extra_opts and overwrite settings when cmd is not
present."""
> +cc_disk_setup.mkfs({
> +'filesystem': 'ext4',
> +'device': '/dev/xdb1',
> +'label': 'without_cmd',
> +'extra_opts': ['are', 'added'],
> +'overwrite': True
> +})
> +
> +subp.assert
Re: [Cloud-init-dev] [Merge] ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master
Review: Approve -- https://code.launchpad.net/~paul-meyer/cloud-init/+git/cloud-init/+merge/323532 Your team cloud init development team is requested to review the proposed merge of ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master
Minor comments inline. -- https://code.launchpad.net/~paul-meyer/cloud-init/+git/cloud-init/+merge/323532 Your team cloud init development team is requested to review the proposed merge of ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
Thanks Ryan for the review points, I know you are commenting on the Safeloader proposal which is over here https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323088 so I'll copy them over to that proposal and comment over there. -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cyaml-loading into cloud-init:master
>From Ryan: Some thoughts on this: 1) we don't want users who apt install cloud-init to also pull down python-devel, gcc and have to compile the extension, so please don't change package deps here 2) this is currently an issue for the tox/venv environment, so let's focus on how to enable the SafeLoader library for that path 3) Once we make (2) an optional testing path (env or parameter can trigger the dep install prior to calling tox/venv which triggers the compile during pip install of the module IIUC), we can have two travis paths, one without the library (representing how things work today) and one *with* the library enabled. This allows us to compare results, unittests all pass, and time it takes to complete (some estimate of the improvement it may provide). 4) With results from (3), we may propose changes to the underlying images (server seed, cloud-image seed, etc) which could ensure that the library is present in the default cloud images. -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323088 Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:cyaml-loading into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cyaml-loading into cloud-init:master
1) We won't have libyaml-dev as a dependency just the shared lib of libyaml-0-2. I've added it as a Recommends and put in logic to handle ImportErrors on CSafeLoader which fall back to SafeLoader 2) I've got a branch up in github which installs tox-needed system debs that we can review as part of the migration to github https://github.com/blackboxsw/cloud-init/pull/3/files 3) I have added a skipUnles decorator on unit tests to ensure we exercise the CSafeLoader path only if that module is present 4) depending on what we want to do with a strict yaml C-binding dependency, we might end up tweaking the debian/control file to just expect this as a mandatory deb (libyaml-0-2). We might need further team discussion on whether that's the approach we want -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323088 Your team cloud init development team is requested to review the proposed merge of ~chad.smith/cloud-init:cyaml-loading into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master
Initial comments added
Diff comments:
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> new file mode 100644
> index 000..00f91f3
> --- /dev/null
> +++ b/tests/unittests/test_ds_identify.py
> @@ -0,0 +1,275 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import copy
> +import os
> +
> +from cloudinit import safeyaml
> +from cloudinit import util
> +from .helpers import CiTestCase, dir2dict, populate_dir
> +
> +UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
> + "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
> +BLKID_EFI_ROOT = """
> +DEVNAME=/dev/sda1
> +UUID=8B36-5390
> +TYPE=vfat
> +PARTUUID=30d7c715-a6ae-46ee-b050-afc6467fc452
> +
> +DEVNAME=/dev/sda2
> +UUID=19ac97d5-6973-4193-9a09-2e6bbfa38262
> +TYPE=ext4
> +PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
> +"""
> +
> +DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
> +DI_DEFAULT_POLICY_NO_DMI = "search,found=all,maybe=all,notfound=disabled"
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s_out='%(out)s'
> +%(name)s_err='%(err)s'
> +%(name)s_ret='%(ret)s'
> +%(name)s() {
> + local out='%(out)s'
> + local err='%(err)s'
> + local ret='%(ret)s'
> + [ "${%(name)s_out}" = "_unset" ] || echo "${%(name)s_out}"
> + [ "${%(name)s_err}" = "_unset" ] || echo "${%(name)s_err}"
> + return ${%(name)s_ret}
> +}
> +"""
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s() {
> + local out='%(out)s' err='%(err)s' r='%(ret)s'
> + [ "$out" = "_unset" ] || echo "$out"
> + [ "$err" = "_unset" ] || echo "$err" 2>&1
> + return $r
> +}
> +"""
> +
> +UUIDS = [
Anything special about these 3 uuids or are they just samples you draw from in
unit tests? If so, we should just use import uuid; uuid.uuid4() local to the
specific unit tests they probably shouldn't be globals.
> +'e7db1e51-3eff-4ce9-8bfd-e8749884034a',
> +'4167a101-a2fc-4071-a078-f9d89b3c548d',
> +'147c295d-f5f4-4d8b-8b90-14682a6ae072',
> +]
> +
> +RC_FOUND = 0
> +RC_NOT_FOUND = 1
> +
> +P_PRODUCT_NAME = "sys/class/dmi/id/product_name"
Maybe we can add a comment about the P_* section
# Paths to contiguration files
> +P_PRODUCT_SERIAL = "sys/class/dmi/id/product_serial"
> +P_PRODUCT_UUID = "sys/class/dmi/id/product_uuid"
> +P_DSID_CFG = "etc/cloud/ds-identify.cfg"
> +
> +
> +class TestDsIdentify(CiTestCase):
> +dsid_path = os.path.realpath('tools/ds-identify')
> +
> +def call(self, rootd=None, mocks=None, args=None, files=None,
> + policy_dmi=DI_DEFAULT_POLICY,
> + policy_nodmi=DI_DEFAULT_POLICY_NO_DMI):
> +if args is None:
> +args = []
> +if mocks is None:
> +mocks = []
> +
> +if files is None:
> +files = {}
> +
> +if rootd is None:
> +rootd = self.tmp_dir()
> +
> +unset = '_unset'
> +wrap = self.tmp_path(path="_shwrap", dir=rootd)
> +populate_dir(rootd, files)
> +
> +# DI_DEFAULT_POLICY* are declared always as to not rely
> +# on the default in the code. This is because SRU releases change
> +# the value in the code, and thus tests would fail there.
> +head = [
> +"DI_MAIN=noop",
nit: Do we have a quote style preference in coud-init (single or doublequotes)?
I'd just like to adhere to one type in general as a general practice, this code
switches between either arbitrarily as does the rest of cloud-init.
> +"DEBUG_LEVEL=2",
> +"DI_LOG=stderr",
> +"PATH_ROOT='%s'" % rootd,
> +". " + self.dsid_path,
> +'DI_DEFAULT_POLICY="%s"' % policy_dmi,
> +'DI_DEFAULT_POLICY_NO_DMI="%s"' % policy_nodmi,
> +""
> +]
> +
> +def write_mock(data):
> +ddata = {'out': None, 'err': None, 'ret': 0}
> +ddata.update(data)
> +for k in ddata:
> +if ddata[k] is None:
> +ddata[k] = unset
> +return SHELL_MOCK_TMPL % ddata
> +
> +mocklines = []
> +defaults = [
> +{'name': 'detect_virt', 'out': None, 'ret': 1},
> +{'name': 'uname', 'out': UNAME_MYSYS},
> +{'name': 'blkid', 'out': BLKID_EFI_ROOT},
> +]
> +
> +written = [d['name'] for d in mocks]
> +for data in mocks:
> +mocklines.append(write_mock(data))
> +for d in defaults:
> +if d['name'] not in written:
> +mocklines.append(write_mock(d))
> +
> +endlines = [
> +'main %s' % ' '.join(['"%s"' % s for s in args])
> +]
> +
> +with open(wrap, "w") as fp:
> +fp.write('\n'.join(head + mocklines + endlines) + "\n")
> +
> +rc = 0
> +try:
> +out, err = util.subp(['sh', '-c', '. %s' % wrap], capture=True)
> +except util.ProcessExecutionError as e:
> +rc = e.exit_code
> +
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master
Diff comments:
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> new file mode 100644
> index 000..00f91f3
> --- /dev/null
> +++ b/tests/unittests/test_ds_identify.py
> @@ -0,0 +1,275 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import copy
> +import os
> +
> +from cloudinit import safeyaml
> +from cloudinit import util
> +from .helpers import CiTestCase, dir2dict, populate_dir
> +
> +UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
> + "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
> +BLKID_EFI_ROOT = """
> +DEVNAME=/dev/sda1
> +UUID=8B36-5390
> +TYPE=vfat
> +PARTUUID=30d7c715-a6ae-46ee-b050-afc6467fc452
> +
> +DEVNAME=/dev/sda2
> +UUID=19ac97d5-6973-4193-9a09-2e6bbfa38262
> +TYPE=ext4
> +PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
> +"""
> +
> +DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
> +DI_DEFAULT_POLICY_NO_DMI = "search,found=all,maybe=all,notfound=disabled"
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s_out='%(out)s'
> +%(name)s_err='%(err)s'
> +%(name)s_ret='%(ret)s'
> +%(name)s() {
> + local out='%(out)s'
> + local err='%(err)s'
> + local ret='%(ret)s'
> + [ "${%(name)s_out}" = "_unset" ] || echo "${%(name)s_out}"
> + [ "${%(name)s_err}" = "_unset" ] || echo "${%(name)s_err}"
> + return ${%(name)s_ret}
> +}
> +"""
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s() {
> + local out='%(out)s' err='%(err)s' r='%(ret)s'
> + [ "$out" = "_unset" ] || echo "$out"
> + [ "$err" = "_unset" ] || echo "$err" 2>&1
> + return $r
> +}
> +"""
> +
> +UUIDS = [
> +'e7db1e51-3eff-4ce9-8bfd-e8749884034a',
> +'4167a101-a2fc-4071-a078-f9d89b3c548d',
> +'147c295d-f5f4-4d8b-8b90-14682a6ae072',
> +]
> +
> +RC_FOUND = 0
> +RC_NOT_FOUND = 1
> +
> +P_PRODUCT_NAME = "sys/class/dmi/id/product_name"
> +P_PRODUCT_SERIAL = "sys/class/dmi/id/product_serial"
> +P_PRODUCT_UUID = "sys/class/dmi/id/product_uuid"
> +P_DSID_CFG = "etc/cloud/ds-identify.cfg"
> +
> +
> +class TestDsIdentify(CiTestCase):
> +dsid_path = os.path.realpath('tools/ds-identify')
> +
> +def call(self, rootd=None, mocks=None, args=None, files=None,
> + policy_dmi=DI_DEFAULT_POLICY,
> + policy_nodmi=DI_DEFAULT_POLICY_NO_DMI):
> +if args is None:
> +args = []
> +if mocks is None:
> +mocks = []
> +
> +if files is None:
> +files = {}
> +
> +if rootd is None:
> +rootd = self.tmp_dir()
> +
> +unset = '_unset'
> +wrap = self.tmp_path(path="_shwrap", dir=rootd)
> +populate_dir(rootd, files)
> +
> +# DI_DEFAULT_POLICY* are declared always as to not rely
> +# on the default in the code. This is because SRU releases change
> +# the value in the code, and thus tests would fail there.
> +head = [
> +"DI_MAIN=noop",
> +"DEBUG_LEVEL=2",
> +"DI_LOG=stderr",
> +"PATH_ROOT='%s'" % rootd,
> +". " + self.dsid_path,
> +'DI_DEFAULT_POLICY="%s"' % policy_dmi,
> +'DI_DEFAULT_POLICY_NO_DMI="%s"' % policy_nodmi,
> +""
> +]
> +
> +def write_mock(data):
> +ddata = {'out': None, 'err': None, 'ret': 0}
> +ddata.update(data)
> +for k in ddata:
> +if ddata[k] is None:
> +ddata[k] = unset
> +return SHELL_MOCK_TMPL % ddata
> +
> +mocklines = []
> +defaults = [
> +{'name': 'detect_virt', 'out': None, 'ret': 1},
> +{'name': 'uname', 'out': UNAME_MYSYS},
> +{'name': 'blkid', 'out': BLKID_EFI_ROOT},
> +]
> +
> +written = [d['name'] for d in mocks]
> +for data in mocks:
> +mocklines.append(write_mock(data))
> +for d in defaults:
> +if d['name'] not in written:
> +mocklines.append(write_mock(d))
> +
> +endlines = [
> +'main %s' % ' '.join(['"%s"' % s for s in args])
> +]
> +
> +with open(wrap, "w") as fp:
> +fp.write('\n'.join(head + mocklines + endlines) + "\n")
> +
> +rc = 0
> +try:
> +out, err = util.subp(['sh', '-c', '. %s' % wrap], capture=True)
> +except util.ProcessExecutionError as e:
> +rc = e.exit_code
> +out = e.stdout
> +err = e.stderr
> +
> +cfg = None
> +cfg_out = os.path.join(rootd, 'run/cloud-init/cloud.cfg')
> +if os.path.exists(cfg_out):
> +cfg = safeyaml.load(util.load_file(cfg_out))
> +
> +return rc, out, err, cfg, dir2dict(rootd)
> +
> +def _call_via_dict(self, data, rootd=None):
> +# return output of self.call with a dict input like VALID_CFG[item]
> +kwargs = {'rootd': rootd}
> +for k in ('mocks', 'args', 'policy_dmi', 'policy_nodmi', 'files'):
> +
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master
Diff comments:
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> new file mode 100644
> index 000..00f91f3
> --- /dev/null
> +++ b/tests/unittests/test_ds_identify.py
> @@ -0,0 +1,275 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import copy
> +import os
> +
> +from cloudinit import safeyaml
> +from cloudinit import util
> +from .helpers import CiTestCase, dir2dict, populate_dir
> +
> +UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
> + "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
> +BLKID_EFI_ROOT = """
> +DEVNAME=/dev/sda1
> +UUID=8B36-5390
> +TYPE=vfat
> +PARTUUID=30d7c715-a6ae-46ee-b050-afc6467fc452
> +
> +DEVNAME=/dev/sda2
> +UUID=19ac97d5-6973-4193-9a09-2e6bbfa38262
> +TYPE=ext4
> +PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
> +"""
> +
> +DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
> +DI_DEFAULT_POLICY_NO_DMI = "search,found=all,maybe=all,notfound=disabled"
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s_out='%(out)s'
> +%(name)s_err='%(err)s'
> +%(name)s_ret='%(ret)s'
> +%(name)s() {
> + local out='%(out)s'
> + local err='%(err)s'
> + local ret='%(ret)s'
> + [ "${%(name)s_out}" = "_unset" ] || echo "${%(name)s_out}"
> + [ "${%(name)s_err}" = "_unset" ] || echo "${%(name)s_err}"
> + return ${%(name)s_ret}
> +}
> +"""
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s() {
> + local out='%(out)s' err='%(err)s' r='%(ret)s'
> + [ "$out" = "_unset" ] || echo "$out"
> + [ "$err" = "_unset" ] || echo "$err" 2>&1
> + return $r
> +}
> +"""
> +
> +UUIDS = [
> +'e7db1e51-3eff-4ce9-8bfd-e8749884034a',
> +'4167a101-a2fc-4071-a078-f9d89b3c548d',
> +'147c295d-f5f4-4d8b-8b90-14682a6ae072',
> +]
> +
> +RC_FOUND = 0
> +RC_NOT_FOUND = 1
> +
> +P_PRODUCT_NAME = "sys/class/dmi/id/product_name"
> +P_PRODUCT_SERIAL = "sys/class/dmi/id/product_serial"
> +P_PRODUCT_UUID = "sys/class/dmi/id/product_uuid"
> +P_DSID_CFG = "etc/cloud/ds-identify.cfg"
> +
> +
> +class TestDsIdentify(CiTestCase):
> +dsid_path = os.path.realpath('tools/ds-identify')
> +
> +def call(self, rootd=None, mocks=None, args=None, files=None,
> + policy_dmi=DI_DEFAULT_POLICY,
> + policy_nodmi=DI_DEFAULT_POLICY_NO_DMI):
> +if args is None:
> +args = []
> +if mocks is None:
> +mocks = []
> +
> +if files is None:
> +files = {}
> +
> +if rootd is None:
> +rootd = self.tmp_dir()
> +
> +unset = '_unset'
> +wrap = self.tmp_path(path="_shwrap", dir=rootd)
> +populate_dir(rootd, files)
> +
> +# DI_DEFAULT_POLICY* are declared always as to not rely
> +# on the default in the code. This is because SRU releases change
> +# the value in the code, and thus tests would fail there.
> +head = [
> +"DI_MAIN=noop",
> +"DEBUG_LEVEL=2",
> +"DI_LOG=stderr",
> +"PATH_ROOT='%s'" % rootd,
> +". " + self.dsid_path,
> +'DI_DEFAULT_POLICY="%s"' % policy_dmi,
> +'DI_DEFAULT_POLICY_NO_DMI="%s"' % policy_nodmi,
> +""
> +]
> +
> +def write_mock(data):
> +ddata = {'out': None, 'err': None, 'ret': 0}
> +ddata.update(data)
> +for k in ddata:
> +if ddata[k] is None:
> +ddata[k] = unset
> +return SHELL_MOCK_TMPL % ddata
> +
> +mocklines = []
> +defaults = [
> +{'name': 'detect_virt', 'out': None, 'ret': 1},
> +{'name': 'uname', 'out': UNAME_MYSYS},
> +{'name': 'blkid', 'out': BLKID_EFI_ROOT},
> +]
> +
> +written = [d['name'] for d in mocks]
> +for data in mocks:
> +mocklines.append(write_mock(data))
> +for d in defaults:
> +if d['name'] not in written:
> +mocklines.append(write_mock(d))
> +
> +endlines = [
> +'main %s' % ' '.join(['"%s"' % s for s in args])
> +]
> +
> +with open(wrap, "w") as fp:
> +fp.write('\n'.join(head + mocklines + endlines) + "\n")
> +
> +rc = 0
> +try:
> +out, err = util.subp(['sh', '-c', '. %s' % wrap], capture=True)
> +except util.ProcessExecutionError as e:
> +rc = e.exit_code
> +out = e.stdout
> +err = e.stderr
> +
> +cfg = None
> +cfg_out = os.path.join(rootd, 'run/cloud-init/cloud.cfg')
> +if os.path.exists(cfg_out):
> +cfg = safeyaml.load(util.load_file(cfg_out))
> +
> +return rc, out, err, cfg, dir2dict(rootd)
> +
> +def _call_via_dict(self, data, rootd=None):
> +# return output of self.call with a dict input like VALID_CFG[item]
> +kwargs = {'rootd': rootd}
> +for k in ('mocks', 'args', 'policy_dmi', 'policy_nodmi', 'files'):
> +
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master
Review: Approve
+1 thanks for the rework + docstrings.
Diff comments:
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> new file mode 100644
> index 000..de645eb
> --- /dev/null
> +++ b/tests/unittests/test_ds_identify.py
> @@ -0,0 +1,295 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import copy
> +import os
> +
> +from cloudinit import safeyaml
> +from cloudinit import util
> +from .helpers import CiTestCase, dir2dict, json_dumps, populate_dir
> +
> +UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
> + "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
> +BLKID_EFI_ROOT = """
> +DEVNAME=/dev/sda1
> +UUID=8B36-5390
> +TYPE=vfat
> +PARTUUID=30d7c715-a6ae-46ee-b050-afc6467fc452
> +
> +DEVNAME=/dev/sda2
> +UUID=19ac97d5-6973-4193-9a09-2e6bbfa38262
> +TYPE=ext4
> +PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
> +"""
> +
> +DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
> +DI_DEFAULT_POLICY_NO_DMI = "search,found=all,maybe=all,notfound=disabled"
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s() {
> + local out='%(out)s' err='%(err)s' r='%(ret)s' RET='%(RET)s'
> + [ "$out" = "_unset" ] || echo "$out"
> + [ "$err" = "_unset" ] || echo "$err" 2>&1
> + [ "$RET" = "_unset" ] || _RET="$RET"
> + return $r
> +}
> +"""
> +
> +UUIDS = [
> +'e7db1e51-3eff-4ce9-8bfd-e8749884034a',
uuid.uuid4() in the ConfigDrive definition and we're good to go.
> +'4167a101-a2fc-4071-a078-f9d89b3c548d',
> +'147c295d-f5f4-4d8b-8b90-14682a6ae072',
> +]
> +
> +RC_FOUND = 0
> +RC_NOT_FOUND = 1
> +DS_NONE = 'None'
> +
> +P_PRODUCT_NAME = "sys/class/dmi/id/product_name"
> +P_PRODUCT_SERIAL = "sys/class/dmi/id/product_serial"
> +P_PRODUCT_UUID = "sys/class/dmi/id/product_uuid"
> +P_DSID_CFG = "etc/cloud/ds-identify.cfg"
> +
> +MOCK_VIRT_IS_KVM = {'name': 'detect_virt', 'RET': 'kvm', 'ret': 0}
> +
> +
> +class TestDsIdentify(CiTestCase):
> +dsid_path = os.path.realpath('tools/ds-identify')
> +
> +def call(self, rootd=None, mocks=None, args=None, files=None,
> + policy_dmi=DI_DEFAULT_POLICY,
> + policy_nodmi=DI_DEFAULT_POLICY_NO_DMI):
> +if args is None:
> +args = []
> +if mocks is None:
> +mocks = []
> +
> +if files is None:
> +files = {}
> +
> +if rootd is None:
> +rootd = self.tmp_dir()
> +
> +unset = '_unset'
> +wrap = self.tmp_path(path="_shwrap", dir=rootd)
> +populate_dir(rootd, files)
> +
> +# DI_DEFAULT_POLICY* are declared always as to not rely
> +# on the default in the code. This is because SRU releases change
> +# the value in the code, and thus tests would fail there.
> +head = [
> +"DI_MAIN=noop",
> +"DEBUG_LEVEL=2",
> +"DI_LOG=stderr",
> +"PATH_ROOT='%s'" % rootd,
> +". " + self.dsid_path,
> +'DI_DEFAULT_POLICY="%s"' % policy_dmi,
> +'DI_DEFAULT_POLICY_NO_DMI="%s"' % policy_nodmi,
> +""
> +]
> +
> +def write_mock(data):
> +ddata = {'out': None, 'err': None, 'ret': 0, 'RET': None}
> +ddata.update(data)
> +for k in ddata:
> +if ddata[k] is None:
> +ddata[k] = unset
> +return SHELL_MOCK_TMPL % ddata
> +
> +mocklines = []
> +defaults = [
> +{'name': 'detect_virt', 'RET': 'none', 'ret': 1},
> +{'name': 'uname', 'out': UNAME_MYSYS},
> +{'name': 'blkid', 'out': BLKID_EFI_ROOT},
> +]
> +
> +written = [d['name'] for d in mocks]
> +for data in mocks:
> +mocklines.append(write_mock(data))
> +for d in defaults:
> +if d['name'] not in written:
> +mocklines.append(write_mock(d))
> +
> +endlines = [
> +'main %s' % ' '.join(['"%s"' % s for s in args])
> +]
> +
> +with open(wrap, "w") as fp:
> +fp.write('\n'.join(head + mocklines + endlines) + "\n")
> +
> +rc = 0
> +try:
> +out, err = util.subp(['sh', '-c', '. %s' % wrap], capture=True)
> +except util.ProcessExecutionError as e:
> +rc = e.exit_code
> +out = e.stdout
> +err = e.stderr
> +
> +cfg = None
> +cfg_out = os.path.join(rootd, 'run/cloud-init/cloud.cfg')
> +if os.path.exists(cfg_out):
> +contents = util.load_file(cfg_out)
> +try:
> +cfg = safeyaml.load(contents)
> +except Exception as e:
> +cfg = {"_INVALID_YAML": contents,
> + "_EXCEPTION": str(e)}
> +
> +return rc, out, err, cfg, dir2dict(rootd)
> +
> +def _call_via_dict(self, data, rootd=None, **kwargs):
> +# return output of self.call with a dict input li
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:sysconfig-has-default into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:sysconfig-has-default
into cloud-init:master.
Requested reviews:
cloud init development team (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323819
sysconfig: Properly set Route.has_set_default_(ipv4|ipv6) when processing
default gateways.
This fixes a regression when Route class definition changed to track ipv6 vs
ipv4 default gateways. Added unit tests to exercise the intended logic which
would raise a ValueError if multiple default gateway for ipv4 or ipv6 were
found.
--
Your team cloud init development team is requested to review the proposed merge
of ~chad.smith/cloud-init:sysconfig-has-default into cloud-init:master.
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 504e4d0..d981277 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -232,12 +232,8 @@ class Renderer(renderer.Renderer):
iface_cfg.name))
if 'netmask' in subnet:
iface_cfg['NETMASK'] = subnet['netmask']
+is_ipv6 = subnet.get('ipv6')
for route in subnet.get('routes', []):
-if subnet.get('ipv6'):
-gw_cfg = 'IPV6_DEFAULTGW'
-else:
-gw_cfg = 'GATEWAY'
-
if _is_default_route(route):
if (
(subnet.get('ipv4') and
@@ -258,8 +254,12 @@ class Renderer(renderer.Renderer):
# also provided the default route?
iface_cfg['DEFROUTE'] = True
if 'gateway' in route:
-iface_cfg[gw_cfg] = route['gateway']
-route_cfg.has_set_default = True
+if is_ipv6:
+iface_cfg['IPV6_DEFAULTGW'] = route['gateway']
+route_cfg.has_set_default_ipv6 = True
+else:
+iface_cfg['GATEWAY'] = route['gateway']
+route_cfg.has_set_default_ipv4 = True
else:
gw_key = 'GATEWAY%s' % route_cfg.last_idx
nm_key = 'NETMASK%s' % route_cfg.last_idx
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 89e7536..eec4159 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -880,6 +880,82 @@ USERCTL=no
""".lstrip()
self.assertEqual(expected_content, content)
+def test_multiple_ipv4_default_gateways(self):
+"""ValueError is raised when duplicate ipv4 gateways exist."""
+net_json = {
+"services": [{"type": "dns", "address": "172.19.0.12"}],
+"networks": [{
+"network_id": "dacd568d-5be6-4786-91fe-750c374b78b4",
+"type": "ipv4", "netmask": "255.255.252.0",
+"link": "tap1a81968a-79",
+"routes": [{
+"netmask": "0.0.0.0",
+"network": "0.0.0.0",
+"gateway": "172.19.3.254",
+}, {
+"netmask": "0.0.0.0", # A second default gateway
+"network": "0.0.0.0",
+"gateway": "172.20.3.254",
+}],
+"ip_address": "172.19.1.34", "id": "network0"
+}],
+"links": [
+{
+"ethernet_mac_address": "fa:16:3e:ed:9a:59",
+"mtu": None, "type": "bridge", "id":
+"tap1a81968a-79",
+"vif_id": "1a81968a-797a-400f-8a80-567f997eb93f"
+},
+],
+}
+macs = {'fa:16:3e:ed:9a:59': 'eth0'}
+render_dir = self.tmp_dir()
+network_cfg = openstack.convert_net_json(net_json, known_macs=macs)
+ns = network_state.parse_net_config_data(network_cfg,
+ skip_broken=False)
+renderer = sysconfig.Renderer()
+with self.assertRaises(ValueError):
+renderer.render_network_state(ns, render_dir)
+self.assertEqual([], os.listdir(render_dir))
+
+def test_multiple_ipv6_default_gateways(self):
+"""ValueError is raised when duplicate ipv6 gateways exist."&q
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
initial doc notes. haven't gotten to the code yet. This looks good/very helpful. Diff comments: > diff --git a/doc/rtd/topics/tests.rst b/doc/rtd/topics/tests.rst > index 0663811..0e470c6 100644 > --- a/doc/rtd/topics/tests.rst > +++ b/doc/rtd/topics/tests.rst > @@ -47,20 +47,29 @@ The test configuration is a YAML file such as > *ntp_server.yaml* below: > cat /etc/ntp.conf | grep '^server' > > > -There are two keys, 1 required and 1 optional, in the YAML file: > +There are several keys, 1 required and some optional, in the YAML file: > > 1. The required key is ``cloud_config``. This should be a string of valid > YAML that is exactly what would normally be placed in a cloud-config file, > including the cloud-config header. This essentially sets up the scenario > under test. > > -2. The optional key is ``collect_scripts``. This key has one or more > +2. One optional key is ``collect_scripts``. This key has one or more > sub-keys containing strings of arbitrary commands to execute (e.g. > ```cat /var/log/cloud-config-output.log```). In the example above the > output of dpkg is captured, grep for ntp, and the number of lines > reported. The name of the sub-key is important. The sub-key is used by > the verification script to recall the output of the commands ran. > > +3. The optinal key ``enabled`` enables or disables the test case. By defaul s/optinal/optional/ s/defaul/default/ > + the test case will be enabled. > + > +4. The optional key ``required_features`` key may be used to specify a list s/optional key/optional / > + of features flags that an image must have to be able to run the testcase. > + For example, if a testcase relies on an image supporting apt, then the > + config for the testcase should include ``required_features: [ apt ]``. > + > + > Default Collect Scripts > --- > > @@ -252,15 +316,259 @@ configuration users can run the integration tests via > tox: > Users need to invoke the citest enviornment and then pass any additional > arguments. > > +Setup Image > +--- > +The ``run`` and ``collect`` commands have many options to setup the image > +before running tests in addition to installing a deb in the target. Any > +combination of the following can be used: > + > +* ``--deb``: install a deb into the image > +* ``--rpm``: install a rpm into the image > +* ``--repo``: enable a repository and update cloud-init afterwards > +* ``--ppa``: enable a ppa and update cloud-init afterwards > +* ``--upgrade``: upgrade cloud-init from repos > +* ``--upgrade-full``: run a full system upgrade > +* ``--script``: execute a script in the image. this can perform any setup > + required that is not covered by the other options > + > +Configuring the Test Suite > +== > + > +Most of the behavior of the test suite is configurable through several yaml > +files. These control the behavior of the test suite's platforms, images, and > +tests. The main config files for platforms, images and testcases are > +``platforms.yaml``, ``releases.yaml`` and ``testcases.yaml``. > + > +Config handling > +--- > +All configurable parts of the test suite use a defaults + overrides system > for > +managing config entries. All base config items are dictionaries. > + > +Merging is done on a key by key basis, with all keys in the default and > +overrides represented in the final result. If a key exists both in > +the defaults and the overrides, then behavior depends on the type of data the > +key refers to. If it is atomic data or a list, then the overrides will > replace > +the default. If the data is a dictionary then the value will be the result of > +merging that dictionary from the default config and that dictionary from the > +overrides. > + > +Merging is done using the function ``tests.cloud_tests.config.merge_config``, > +which can be examined for more detail on config merging behavior. > + > +The following demonstrates merge behavior: > + > +.. code-block:: yaml > + > +defaults: > +list_item: > + - list_entry_1 > + - list_entry_2 > +int_item_1: 123 > +int_item_2: 234 > +dict_item: > +subkey_1: 1 > +subkey_2: 2 > +subkey_dict: > +subsubkey_1: a > +subsubkey_2: b > + > +overrides: > +list_item: > + - overridden_list_entry > +int_item_1: 0 > +dict_item: > +subkey_2: false > +subkey_dict: > +subsubkey_2: 'new value' > + > +result: > +list_item: > + - overridden_list_entry > +int_item_1: 0 > +int_item_2: 234 > +dict_item: > +subkey_1: 1 > +subkey_2: false > +subkey_dict: > +subsubkey_1: a > +subsubkey_2: 'new value' > + > + > +Image Config Structure > +-- > +Image configurat
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
Diff comments:
> diff --git a/tests/cloud_tests/args.py b/tests/cloud_tests/args.py
> index 371b044..ca52b61 100644
> --- a/tests/cloud_tests/args.py
> +++ b/tests/cloud_tests/args.py
> @@ -185,6 +246,17 @@ def normalize_output_args(args):
> return args
>
>
> +def normalize_output_deb_args(args):
> +"""
Docstrings should be one intro line followed by a blank line and then
additional details per PEP 257 https://www.python.org/dev/peps/pep-0257/
> +normalize OUTPUT_DEB arguments
> +args: parsed args
> +return_value: updated args, or None if erros occurred
> +"""
> +# make sure to use abspath for deb
> +args.deb = os.path.abspath(args.deb)
> +return args
> +
> +
> def normalize_setup_args(args):
> """
> normalize SETUP arguments
> diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py
> new file mode 100644
> index 000..b36331b
> --- /dev/null
> +++ b/tests/cloud_tests/bddeb.py
> @@ -0,0 +1,124 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from tests.cloud_tests import (config, LOG)
> +from tests.cloud_tests import (platforms, images, snapshots, instances)
> +from tests.cloud_tests.stage import (PlatformComponent, run_stage,
> run_single)
> +
> +from cloudinit import util as c_util
> +
> +from functools import partial
> +import os
> +
> +build_deps = ['devscripts', 'equivs', 'git', 'tar']
> +
> +
> +def _out(cmd_res):
> +"""
This can all fit on one line
"""Get clean output from cmd result."""
> +get clean output from cmd result
> +"""
> +return cmd_res[0].strip()
> +
> +
> +def build_deb(args, instance):
> +"""
docstring one line format followed by a blank line and additional details below.
"""Build deb on a remote container, returning it locally to args.deb.
@param args: Command line arguments.
@return_value: tuple of results and failure count.
"""
> +build deb on system and copy out to location at args.deb
> +args: cmdline arguments
> +return_value: tuple of results and fail count
> +"""
> +# update remote system package list and install build deps
> +LOG.debug('installing build deps')
> +pkgs = ' '.join(build_deps)
> +cmd = 'apt-get update && apt-get install --yes {}'.format(pkgs)
> +instance.execute(['/bin/sh', '-c', cmd])
> +instance.execute(['mk-build-deps', '--install', '-t',
If I understand this correctly, this call will break if we add new build
dependencies to cloud-init because we would be installing downstream
(published) cloud-init and not upstream(master) right? I feel like what we are
looking for is a make ci-deps target in cloud-init which we can call after we
push the source_tar over to the remote system. If you agree, can you put an
comment like "# XXX Remove this call once we have a ci-deps Makefile target?
> + 'apt-get --no-install-recommends --yes', 'cloud-init'])
> +
> +# local tmpfile that must be deleted
> +local_tarball = _out(c_util.subp(['mktemp'], capture=True))
> +
> +try:
Instead of calling out to mktemp, we could import tempfile and with
tempfile.NamedTemporaryFile() as local_tarball:
Then you can drop the finally: section and the call to mktemp as that tempfile
is autoremoved when it leaves scope.
> +# paths to use in remote system
> +remote_tarball = _out(instance.execute(['mktemp']))
> +extract_dir = _out(instance.execute(['mktemp', '--directory']))
> +bddeb_path = os.path.join(extract_dir, 'packages', 'bddeb')
> +git_env = {'GIT_DIR': os.path.join(extract_dir, '.git'),
> + 'GIT_WORK_TREE': extract_dir}
> +
> +# create a tarball of cloud init tree and copy to remote system
probably don't need the comment as the log messages explain this well.
> +LOG.debug('creating tarball of cloud-init at: %s', local_tarball)
> +c_util.subp(['tar', 'cf', local_tarball, '--owner', 'root',
> + '--group', 'root', '-C', args.cloud_init, '.'])
> +LOG.debug('copying to remote system at: %s', remote_tarball)
> +instance.push_file(local_tarball, remote_tarball)
> +
> +# extract tarball in remote system and commit anything uncommitted
> +LOG.debug('extracting tarball in remote system at: %s', extract_dir)
> +instance.execute(['tar', 'xf', remote_tarball, '-C', extract_dir])
> +instance.execute(['git', 'commit', '-a', '-m', 'tmp',
> '--allow-empty'],
Not sure we really have to commit anything here, as we can also specify
SKIP_UNCOMITTED_CHANGES_CHECK=1 on the builddeb call. Which also means we can
drop the git_env above I think.
> + env=git_env)
> +
> +# build the deb, ignoring missing deps (flake8)
> +output_link = '/root/cloud-init_all.deb'
> +LOG.debug('building deb in remote system at: %s', output_link)
> +bddeb_args = args.bddeb_args.split() if args.bddeb_ar
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
Maybe it's just a print problem...
tox -e citest -- tree_run -v -n artful -t
tests/cloud_tests/configs/modules/write_files.yaml
2017-05-15 19:34:34,906 - tests.cloud_tests - INFO - acquiring image for os:
xenial
I would've expected artful to be acquired not xenial.
Diff comments:
> diff --git a/tests/cloud_tests/config.py b/tests/cloud_tests/config.py
> index f3a13c9..9a0ad4d 100644
> --- a/tests/cloud_tests/config.py
> +++ b/tests/cloud_tests/config.py
> @@ -61,22 +75,63 @@ def merge_config(base, override):
> return res
>
>
> -def load_platform_config(platform):
> +def merge_feature_groups(feature_conf, feature_groups, overrides):
> +"""
> +combine feature groups and overrides to construct a supported feature
> list
> +feature_conf: feature config from releases.yaml
> +feature_groups: feature groups the release is a member of
> +overrides: overrides specified by the release's config
> +return_value: dict of {feature: true/false} settings
> +"""
> +res = dict().fromkeys(feature_conf['all'])
> +for group in feature_groups:
> +res.update(feature_conf['groups'][group])
> +res.update(overrides)
> +return res
> +
> +
> +def load_platform_config(platform_name, require_enabled=False):
> """
> load configuration for platform
> +platform_name: name of platform to retrieve config for
docstring oneline, blank line then param descriptions etc. preferably with
@param : but if that's too heavyweight, the blankline
before params should suffice.
> +require_enabled: if true, raise error if 'enabled' not True
> +return_value: config dict
> """
> main_conf = c_util.read_conf(PLATFORM_CONF)
> -return merge_config(main_conf.get('default_platform_config'),
> -main_conf.get('platforms')[platform])
> +conf = merge_config(main_conf['default_platform_config'],
> +main_conf['platforms'][platform_name])
> +if require_enabled and not enabled(conf):
> +raise ValueError('Platform is not enabled')
> +return conf
>
>
> -def load_os_config(os_name):
> +def load_os_config(platform_name, os_name, require_enabled=False,
> + feature_overrides={}):
> """
> load configuration for os
> +platform_name: platform name to load os config for
> +os_name: name of os to retrieve config for
> +require_enabled: if true, raise error if 'enabled' not True
> +feature_overrides: feature flag overrides to merge with features config
> +return_value: config dict
> """
> main_conf = c_util.read_conf(RELEASES_CONF)
> -return merge_config(main_conf.get('default_release_config'),
> -main_conf.get('releases')[os_name])
> +default = main_conf['default_release_config']
> +image = main_conf['releases'][os_name]
> +conf = merge_config(merge_config(get(default, 'default'),
> + get(default, platform_name)),
> +merge_config(get(image, 'default'),
> + get(image, platform_name)))
> +
> +feature_conf = main_conf['features']
> +feature_groups = conf.get('feature_groups', [])
> +overrides = merge_config(get(conf, 'features'), feature_overrides)
> +conf['features'] = merge_feature_groups(
> +feature_conf, feature_groups, overrides)
> +
> +if require_enabled and not enabled(conf):
> +raise ValueError('OS is not enabled')
> +return conf
>
>
> def load_test_config(path):
> @@ -87,20 +142,34 @@ def load_test_config(path):
> c_util.read_conf(name_to_path(path)))
>
>
> +def list_feature_flags():
> +"""
> +list all supported feature flags
can be one line
> +"""
> +feature_conf = get(c_util.read_conf(RELEASES_CONF), 'features')
> +return feature_conf.get('all', [])
> +
> +
> def list_enabled_platforms():
> """
> list all platforms enabled for testing
> """
> -platforms = c_util.read_conf(PLATFORM_CONF).get('platforms')
> -return [k for k, v in platforms.items() if v.get('enabled')]
> +platforms = get(c_util.read_conf(PLATFORM_CONF), 'platforms')
> +return [k for k, v in platforms.items() if enabled(v)]
>
>
> -def list_enabled_distros():
> +def list_enabled_distros(platforms):
> """
> -list all distros enabled for testing
> +list all distros enabled for testing on specified platforms
> """
> -releases = c_util.read_conf(RELEASES_CONF).get('releases')
> -return [k for k, v in releases.items() if v.get('enabled')]
> +
> +def platform_has_enabled(config):
> +return any(enabled(merge_config(get(config, 'default'),
> +get(config, platform)))
> + for platform in platforms)
> +
> +releases = get(c_util.read_conf(RELEASES_CONF), 'releases')
> +return [k for k, v in releases.items() if platform_
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
Thanks for the clarification josh. While it feels strange to build a deb on a different target series --build-os, than test series per --os-name, I suppose that use case for that could be in cases where we know we can't build on the target --os-name due to missing deps or something. In those cases we'd have to specify a different --build-os than --os-name. Additional review comments trickling in. I'm nearly done. Diff comments: > diff --git a/doc/rtd/topics/tests.rst b/doc/rtd/topics/tests.rst > index 0663811..0e470c6 100644 > --- a/doc/rtd/topics/tests.rst > +++ b/doc/rtd/topics/tests.rst > @@ -252,15 +316,259 @@ configuration users can run the integration tests via > tox: > Users need to invoke the citest enviornment and then pass any additional > arguments. > > +Setup Image > +--- > +The ``run`` and ``collect`` commands have many options to setup the image > +before running tests in addition to installing a deb in the target. Any > +combination of the following can be used: > + > +* ``--deb``: install a deb into the image > +* ``--rpm``: install a rpm into the image > +* ``--repo``: enable a repository and update cloud-init afterwards > +* ``--ppa``: enable a ppa and update cloud-init afterwards > +* ``--upgrade``: upgrade cloud-init from repos > +* ``--upgrade-full``: run a full system upgrade > +* ``--script``: execute a script in the image. this can perform any setup > + required that is not covered by the other options > + > +Configuring the Test Suite > +== > + > +Most of the behavior of the test suite is configurable through several yaml > +files. These control the behavior of the test suite's platforms, images, and > +tests. The main config files for platforms, images and testcases are > +``platforms.yaml``, ``releases.yaml`` and ``testcases.yaml``. > + > +Config handling > +--- > +All configurable parts of the test suite use a defaults + overrides system > for > +managing config entries. All base config items are dictionaries. > + > +Merging is done on a key by key basis, with all keys in the default and > +overrides represented in the final result. If a key exists both in > +the defaults and the overrides, then behavior depends on the type of data the > +key refers to. If it is atomic data or a list, then the overrides will > replace > +the default. If the data is a dictionary then the value will be the result of > +merging that dictionary from the default config and that dictionary from the > +overrides. > + > +Merging is done using the function ``tests.cloud_tests.config.merge_config``, > +which can be examined for more detail on config merging behavior. > + > +The following demonstrates merge behavior: > + > +.. code-block:: yaml > + > +defaults: > +list_item: > + - list_entry_1 > + - list_entry_2 > +int_item_1: 123 > +int_item_2: 234 > +dict_item: > +subkey_1: 1 > +subkey_2: 2 > +subkey_dict: > +subsubkey_1: a > +subsubkey_2: b > + > +overrides: > +list_item: > + - overridden_list_entry > +int_item_1: 0 > +dict_item: > +subkey_2: false > +subkey_dict: > +subsubkey_2: 'new value' > + > +result: > +list_item: > + - overridden_list_entry > +int_item_1: 0 > +int_item_2: 234 > +dict_item: > +subkey_1: 1 > +subkey_2: false > +subkey_dict: > +subsubkey_1: a > +subsubkey_2: 'new value' > + > + > +Image Config Structure > +-- > +Image configuration is handled in ``releases.yaml``. The image configuration > +controls how platforms locate and acquire images, how the platforms should > +interact with the images, how platforms should detect when an image has fully > +booted, any options that are required to set the image up, and features that > +the image supports. > + > +Since settings for locating an image and interacting with it differ from > +platform to platform, there are 4 levels of settings available for images on > +top of the default image settings. The structure of the image config file is: > + > +.. code-block:: yaml > + > +default_release_config: > +default: > +... > +: > +... > +: > +... > + > +releases: > +: > +: > +... > +: > +... > +: > +... > + > + > +The base config is created from the overall defaults and the overrides for > the > +platform. The overrides are created from the default config for the image and > +the platform specific overrides for the image. > + > +Image Config for System Boot > + > +The test suite must be able to test if a system has fully booted and if > +cloud-init has finished running, so that r
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
Think I'm done with this branch. Thanks Josh for shepherding this all in.
This is good step in the right direction, and since it's in our test code
instead of core, I shouldn't try to be a stickler on some of these concerns as
we can iterate on this without direct impact to core code.
Diff comments:
> diff --git a/tests/cloud_tests/setup_image.py
> b/tests/cloud_tests/setup_image.py
> index 5d6c638..1b74ceb 100644
> --- a/tests/cloud_tests/setup_image.py
> +++ b/tests/cloud_tests/setup_image.py
> @@ -7,6 +7,30 @@ from functools import partial
> import os
>
>
> +def installed_version(image, package, ensure_installed=True):
rename request to give more context
s/installed_version/installed_package_version/. The benefit of a more verbose
function naming is that it potentially prevents someone from having to come
back from callsites to the original function defintion to find out what the
intent is.
> +"""
> +get installed version of package
> +image: cloud_tests.images instance to operate on
> +package: name of package
> +ensure_installed: raise error if not installed
> +return_value: cloud-init version string
> +"""
> +# get right cmd for os family
> +os_family = util.get_os_family(image.properties['os'])
> +if os_family == 'debian':
> +cmd = ['dpkg-query', '-W', "--showformat='${Version}'", package]
> +elif os_family == 'redhat':
> +cmd = ['rpm', '-q', '--queryformat', "'%{VERSION}'", package]
> +else:
> +raise NotImplementedError
> +
> +# query version
> +msg = 'query version for package: {}'.format(package)
> +(out, err, exit) = image.execute(
> +cmd, description=msg, rcs=(0,) if ensure_installed else range(0,
> 256))
> +return out.strip()
> +
> +
> def install_deb(args, image):
> """
> install deb into image
> diff --git a/tests/cloud_tests/util.py b/tests/cloud_tests/util.py
> index 64a8667..79ed883 100644
> --- a/tests/cloud_tests/util.py
> +++ b/tests/cloud_tests/util.py
> @@ -158,6 +174,140 @@ def write_file(*args, **kwargs):
> """
> write a file using cloudinit.util.write_file
> """
> -c_util.write_file(*args, **kwargs)
> +return c_util.write_file(*args, **kwargs)
> +
> +
> +def read_conf(*args, **kwargs):
> +"""
> +read configuration using cloudinit.util.read_conf
> +"""
> +return c_util.read_conf(*args, **kwargs)
> +
> +
> +def subp(*args, **kwargs):
> +"""
> +execute a command on the system shell using cloudinit.util.subp
> +"""
> +return c_util.subp(*args, **kwargs)
> +
> +
> +def tmpdir(prefix='cloud_test_util_'):
Questionable utility function. We probbably should just call this
tempfile.mkdtemp directly. It's just adding bloat in my mind.
> +return tempfile.mkdtemp(prefix=prefix)
> +
> +
> +def rel_files(basedir):
> +"""
> +list of files under directory by relative path, not including directories
> +return_value: list or relative paths
> +"""
> +basedir = os.path.normpath(basedir)
> +return [path[len(basedir) + 1:] for path in
> +glob.glob(os.path.join(basedir, '**'), recursive=True)
> +if not os.path.isdir(path)]
> +
> +
> +def flat_tar(output, basedir, owner='root', group='root'):
> +"""
> +create a flat tar archive (no leading ./) from basedir
> +output: output tar file to write
> +basedir: base directory for archive
> +owner: owner of archive files
> +group: group archive files belong to
> +return_value: none
> +"""
> +c_util.subp(['tar', 'cf', output, '--owner', owner, '--group', group,
> + '-C', basedir] + rel_files(basedir), capture=True)
> +
> +
> +def parse_conf_list(entries, valid=None, boolean=False):
We can drop boolean if we make this util function a bit smarter. It should
probably also return an empty dict upon invalid keys as it shouldn't change the
type of it's return value. Also it should raise a ValueError if it doesn't see
an = sign so we get a better traceback if called improperly (like
--feature-override asdf:true)
It's almost there:
def parse_conf_list(entries, valid=None):
"""Parse config list of key=value.
entries: List of key=value or key:value configuration strings.
valid: List of valid keys in result.
return_value: Dict of configuration values or empty dict if invalid keys
"""
res = {}
for entry in entries:
if "=" not in entry:
raise ValueError("Invalid key provided. Expected key=value, found
'{}'".format(entry))
key, value = entry.split("=")
if value.lower() in ("true", "false"):
value = value.lower() == 'true'
if valid and key in valid:
res[key] = value
return res
> +"""
> +parse config in a list of strings in key=value format
> +entries: list of key=value strings
> +valid: list of valid keys in result, return None if invalid input
> +boolean: if true, then interpret all va
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master has been updated. Commit Message changed to: make deb: Add devscripts dependency for make deb. Cleanup packages/bddeb. Add a simple dependency check to "make deb" target for devscripts. Rework a bit of the logic in package/bddeb to drop superfluous STD_NAMED_PACKAGES to avoid duplication of requirements already listed in (test-)?requirements.txt. All "standard" packages can be assumed to have either python3- or python- prefix if not listed in NONSTD_NAMED_PACKAGES. This branch also moves logic inside write_debian_folder which is unneeded up in main. Moving the logic inside write_debian_folder helps cut down on the number of parameters we need to paas into the function. LP: #1685935 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/323076 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:make-deb-cleanup into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:integration-test-revamp into cloud-init:master
Review: Approve
Wow thanks for the epic docstring rewrite across all integration tests.
It really is a lot easier to read.
I think all of my comments on the previous branch have been addressed. thank
you.
Since the visual diff is so large it isn't fully rendered here.
Couple inline comments remain and
=I can't comment inline for the following:
tests/cloud_tests/config.py:
- s/[Ss]anatize/[Ss]anitize/
tests/cloud_tests/setup_image.py
- s/#return_value/@return_value/
tests/cloud_tests/util.py:
- s/Utilies/Utilities/ in module docstring
- From a previous branch comment, I still think the tmpdir function defined
here is has limited benefit in redefining a wrapper that passes a
prefix='cloud_test_util', why don't we just call
tempfile.mkdtemp(prefix='cloud_test_util') in the two call sites in
tests/cloud_tests/images/lxd.py?
+1 after that set of changes. Thanks for this herculean effort. With such a
large diff it's hard to catch all the issues effectively. But, since it's
integration testing I'm not worried that we can iron out anything that remains
on subsequent passes/iterations.
Diff comments:
> diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py
> new file mode 100644
> index 000..53dbf74
> --- /dev/null
> +++ b/tests/cloud_tests/bddeb.py
> @@ -0,0 +1,118 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""Used to build a deb."""
> +
> +from functools import partial
> +import os
> +import tempfile
> +
> +from cloudinit import util as c_util
> +from tests.cloud_tests import (config, LOG)
> +from tests.cloud_tests import (platforms, images, snapshots, instances)
> +from tests.cloud_tests.stage import (PlatformComponent, run_stage,
> run_single)
> +
> +build_deps = ['devscripts', 'equivs', 'git', 'tar']
> +
> +
> +def _out(cmd_res):
> +"""Get clean output from cmd result."""
> +return cmd_res[0].strip()
> +
> +
> +def build_deb(args, instance):
> +"""Build deb on system and copy out to location at args.deb.
> +
> +@param args: cmdline arguments
> +@return_value: tuple of results and fail count
> +"""
> +# update remote system package list and install build deps
> +LOG.debug('installing build deps')
> +pkgs = ' '.join(build_deps)
> +cmd = 'apt-get update && apt-get install --yes {}'.format(pkgs)
> +instance.execute(['/bin/sh', '-c', cmd])
> +# TODO Remove this call once we have a ci-deps Makefile target
> +instance.execute(['mk-build-deps', '--install', '-t',
> + 'apt-get --no-install-recommends --yes', 'cloud-init'])
> +
> +# local tmpfile that must be deleted
> +local_tarball = tempfile.NamedTemporaryFile().name
It looks like we aren't deleting this right? Should it be something like the
following instead:
with tempfile.NamedTemporaryFile() as local_tarfile:
... the rest of the code indented within this context and
s/local_tarball/local_tarball.name/
> +
> +# paths to use in remote system
> +output_link = '/root/cloud-init_all.deb'
> +remote_tarball = _out(instance.execute(['mktemp']))
> +extract_dir = _out(instance.execute(['mktemp', '--directory']))
> +bddeb_path = os.path.join(extract_dir, 'packages', 'bddeb')
> +git_env = {'GIT_DIR': os.path.join(extract_dir, '.git'),
> + 'GIT_WORK_TREE': extract_dir}
> +
> +LOG.debug('creating tarball of cloud-init at: %s', local_tarball)
> +c_util.subp(['tar', 'cf', local_tarball, '--owner', 'root',
> + '--group', 'root', '-C', args.cloud_init, '.'])
> +LOG.debug('copying to remote system at: %s', remote_tarball)
> +instance.push_file(local_tarball, remote_tarball)
> +
> +LOG.debug('extracting tarball in remote system at: %s', extract_dir)
> +instance.execute(['tar', 'xf', remote_tarball, '-C', extract_dir])
> +instance.execute(['git', 'commit', '-a', '-m', 'tmp', '--allow-empty'],
> + env=git_env)
> +
> +LOG.debug('building deb in remote system at: %s', output_link)
> +bddeb_args = args.bddeb_args.split() if args.bddeb_args else []
> +instance.execute([bddeb_path, '-d'] + bddeb_args, env=git_env)
> +
> +# copy the deb back to the host system
> +LOG.debug('copying built deb to host at: %s', args.deb)
> +instance.pull_file(output_link, args.deb)
> +
> +
> +def setup_build(args):
> +"""Set build system up then run build.
> +
> +@param args: cmdline arguments
> +@return_value: tuple of results and fail count
> +"""
> +res = ({}, 1)
> +
> +# set up platform
> +LOG.info('setting up platform: %s', args.build_platform)
> +platform_config = config.load_platform_config(args.build_platform)
> +platform_call = partial(platforms.get_platform, args.build_platform,
> +platform_config)
> +with PlatformComponent(platform_call) as platform:
> +
> +# set up image
> +LOG.info('acquiring image for os: %
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:pylint/1444-pylint-tip-ignore-e1101-from-contextlib into cloud-init:master
s/pylint-tip/tip-pylint/ in commit message.
Diff comments:
> diff --git a/setup.py b/setup.py
> index 4616599..a61c24a 100755
> --- a/setup.py
> +++ b/setup.py
> @@ -76,6 +77,9 @@ INITSYS_ROOTS = {
> 'systemd': pkg_config_read('systemd', 'systemdsystemunitdir'),
> 'systemd.generators': pkg_config_read('systemd',
>'systemdsystemgeneratordir'),
> +'systemd.fsck-dropin': (
> +os.path.sep.join([pkg_config_read('systemd', 'systemdsystemunitdir'),
Why not just use os.path.join(pkg_config_read('systemd',
'systemdsystemunitdir'),
'systemd-fsck@.service.d')?
I just haven't seen os.path.sep used very often.
> + 'systemd-fsck@.service.d'])),
> 'upstart': '/etc/init/',
> }
> INITSYS_TYPES = sorted([f.partition(".")[0] for f in INITSYS_ROOTS.keys()])
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324401
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:pylint/1444-pylint-tip-ignore-e1101-from-contextlib into
cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:pylint/1444-pylint-tip-ignore-e1101-from-contextlib into cloud-init:master
Review: Approve
I think we agreed in standup that since you've fixed this with a workaround for
pylint's bug, you could probably add a structured comment or something that
mark that we should look at pylint tip in the future to see if that issue is
fixed. For bugs on external dependencies which cloud-init expects to be fixed
at some in the future, we probably should decide on a comment label or
something if we need to revisit this logic in the future to clean it up or
remove a workaround. (like # RELEASE_REVIEW or RELEASE_BLOCKER). Then we can
have a simple grep to search for fixes or old external bugs we should double
check upon next SRU.
Diff comments:
> diff --git a/setup.py b/setup.py
> index 4616599..a61c24a 100755
> --- a/setup.py
> +++ b/setup.py
> @@ -76,6 +77,9 @@ INITSYS_ROOTS = {
> 'systemd': pkg_config_read('systemd', 'systemdsystemunitdir'),
> 'systemd.generators': pkg_config_read('systemd',
>'systemdsystemgeneratordir'),
> +'systemd.fsck-dropin': (
> +os.path.sep.join([pkg_config_read('systemd', 'systemdsystemunitdir'),
Chatted in IRC, os.path.sep.join is more defensive in the event that the 2nd
arg is already prefixed by a leading /.
> + 'systemd-fsck@.service.d'])),
> 'upstart': '/etc/init/',
> }
> INITSYS_TYPES = sorted([f.partition(".")[0] for f in INITSYS_ROOTS.keys()])
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324401
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:pylint/1444-pylint-tip-ignore-e1101-from-contextlib into
cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments: > diff --git a/cloudinit/sources/DataSourceEc2.py > b/cloudinit/sources/DataSourceEc2.py > index 2f9c7ed..818a639 100644 > --- a/cloudinit/sources/DataSourceEc2.py > +++ b/cloudinit/sources/DataSourceEc2.py > @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval): > return mode, sleep > > > +def _is_explicit_dslist(dslist): > +if 'Ec2' not in dslist: Should we support case-insensitivity for the configured datasources? ec2 EC2 Ec2? > +return False > +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist)) > + > + > def warn_if_necessary(cfgval, cfg): > try: > mode, sleep = parse_strict_mode(cfgval) -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 2f9c7ed..818a639 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> return mode, sleep
>
>
> +def _is_explicit_dslist(dslist):
bikeshed disclaimer: _is_explicit_dslist is really only checking if Ec2 is
explicitly configured, why not rename _is_ec2_explicit_datasource? Then there
is no question about what this function does.
> +if 'Ec2' not in dslist:
Also, is it possible that dslist comes in configured as None via either an
improper "datasource_list: " or a config.cfg that doesn't define a
datasource_list at all so our (self.sys_cfg.get('datasource_list') gives us
None?
If so, then our 'Ec2' in dslist check would stacktrace
> +return False
> +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
> +
> +
> def warn_if_necessary(cfgval, cfg):
> try:
> mode, sleep = parse_strict_mode(cfgval)
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 2f9c7ed..818a639 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
There only seems to be one unit test which covers this module
(tests/unittests/test_datasource/test_aliyun.py). Is that right? Since we are
adding some level of functionality it'd be nice to cover this change in
functional behavior somewhere.
> @@ -64,7 +64,8 @@ class DataSourceEc2(sources.DataSource):
> LOG.debug("strict_mode: %s, cloud_platform=%s",
>strict_mode, self.cloud_platform)
> if strict_mode == "true" and self.cloud_platform ==
> Platforms.UNKNOWN:
> -return False
> +if not _is_explicit_dslist(self.sys_cfg.get('datasource_list')):
> +return False
>
> try:
> if not self.wait_for_metadata_service():
> @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> return mode, sleep
>
>
> +def _is_explicit_dslist(dslist):
> +if 'Ec2' not in dslist:
> +return False
> +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
> +
> +
> def warn_if_necessary(cfgval, cfg):
> try:
> mode, sleep = parse_strict_mode(cfgval)
> @@ -276,6 +283,12 @@ def warn_if_necessary(cfgval, cfg):
> if mode == "false":
> return
>
> +dslist = cfg.get('datasource_list')
> +if _is_explicit_dslist(cfg.get('datasource_list')):
> +LOG.debug("mode=%s but explicit dslist (%s), not warning.",
> + mode, dslist)
> +return
> +
> warnings.show_warning('non_ec2_md', cfg, mode=True, sleep=sleep)
>
>
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> index 9e14885..8559e1f 100644
> --- a/tests/unittests/test_ds_identify.py
> +++ b/tests/unittests/test_ds_identify.py
> @@ -210,6 +210,13 @@ class TestDsIdentify(CiTestCase):
> mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n'
> self._check_via_dict(mydata, rc=RC_FOUND, dslist=['NoCloud',
> DS_NONE])
>
> +def test_configured_list_with_none(self):
> +"""If user set a datasource_list, that should be used."""
This docstring should really be
"When a datasource_list already contains None, None is not added to the list.
The test just above it is already asserting datasource_list w/out None results
in a list w/ None at the end.
> +mydata = copy.deepcopy(VALID_CFG['GCE'])
> +cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg'
> +mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n'
> +self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE])
> +
>
> def blkid_out(disks=None):
> """Convert a list of disk dictionaries into blkid content."""
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master has been updated. Description changed to: cc_ntp: Restructure cc_ntp unit tests add with_logs class attr to unit tests. Any CiTestCase subclass can now set a class attribute with_logs = True and tests can now make assertions on self.logs.getvalue(). This branch restructures a bit of cc_ntp module to get better test coverage of the module. It also restructures the handler_cc_ntp unit tests to avoid nested mocks where possible. Deeply nested mocks cause a couple of issues: - greater risk: mocks are permanent within the scope, so multiple call-sites could be affected by package mocks - less legible tests: each mock doesn't advertise the actual call-site - tight coupling: the unit test logic to tightly bound to the actual implementation in remote (unrelated) modules which makes it more costly to maintain code - false success: we should be testing the expected behavior not specific remote method names as we want to know if that underlying behavior changes and breaks us. LP: # 1692794 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master has been updated. Commit Message changed to: cc_ntp: Restructure cc_ntp unit tests add with_logs class attr to unit tests. Any CiTestCase subclass can now set a class attribute with_logs = True and tests can now make assertions on self.logs.getvalue(). This branch restructures a bit of cc_ntp module to get better test coverage of the module. It also restructures the handler_cc_ntp unit tests to avoid nested mocks where possible. Deeply nested mocks cause a couple of issues: - greater risk: mocks are permanent within the scope, so multiple call-sites could be affected by package mocks - less legible tests: each mock doesn't advertise the actual call-site - tight coupling: the unit test logic to tightly bound to the actual implementation in remote (unrelated) modules which makes it more costly to maintain code - false success: we should be testing the expected behavior not specific remote method names as we want to know if that underlying behavior changes and breaks us. LP: # 1692794 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:cc-ntp-testing into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450
cc_ntp: Restructure cc_ntp unit tests add with_logs class attr to unit tests.
Any CiTestCase subclass can now set a class attribute with_logs = True and
tests can now make assertions on self.logs.getvalue(). This branch restructures
a bit of cc_ntp module to get better test coverage of the module. It also
restructures the handler_cc_ntp unit tests to avoid nested mocks where
possible. Deeply nested mocks cause a couple of issues:
- greater risk: mocks are permanent within the scope, so multiple call-sites
could be affected by package mocks
- less legible tests: each mock doesn't advertise the actual call-site
- tight coupling: the unit test logic to tightly bound to the actual
implementation in remote (unrelated) modules which makes it more costly to
maintain code
- false success: we should be testing the expected behavior not specific
remote method names as we want to know if that underlying behavior changes and
breaks us.
LP: # 1692794
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master.
diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 225f898..5b31b51 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -39,7 +39,6 @@ servers or pools are provided, 4 pools will be used in the format
from cloudinit import log as logging
from cloudinit.settings import PER_INSTANCE
from cloudinit import templater
-from cloudinit import type_utils
from cloudinit import util
import os
@@ -53,14 +52,12 @@ distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
def handle(name, cfg, cloud, log, _args):
-"""
-Enable and configure ntp
+"""Enable and configure ntp."""
-ntp:
- pools: ['0.{{distro}}.pool.ntp.org', '1.{{distro}}.pool.ntp.org']
- servers: ['192.168.2.1']
-
-"""
+if 'ntp' not in cfg:
+LOG.debug(
+"Skipping module named %s, not present or disabled by cfg", name)
+return
ntp_cfg = cfg.get('ntp', {})
@@ -69,18 +66,12 @@ def handle(name, cfg, cloud, log, _args):
" but not a dictionary type,"
" is a %s %instead"), type_utils.obj_name(ntp_cfg))
-if 'ntp' not in cfg:
-LOG.debug("Skipping module named %s,"
- "not present or disabled by cfg", name)
-return True
-
rename_ntp_conf()
# ensure when ntp is installed it has a configuration file
# to use instead of starting up with packaged defaults
-write_ntp_config_template(ntp_cfg, cloud)
+write_ntp_config_template(cfg['ntp'], cloud)
install_ntp(cloud.distro.install_packages, packages=['ntp'],
check_exe="ntpd")
-
# if ntp was already installed, it may not have started
try:
reload_ntp(systemd=cloud.distro.uses_systemd())
@@ -98,8 +89,10 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"):
install_func(packages)
-def rename_ntp_conf(config=NTP_CONF):
+def rename_ntp_conf(config=None):
"""Rename any existing ntp.conf file and render from template"""
+if config is None: # For testing
+config = NTP_CONF
if os.path.exists(config):
util.rename(config, config + ".dist")
@@ -117,8 +110,10 @@ def write_ntp_config_template(cfg, cloud):
pools = cfg.get('pools', [])
if len(servers) == 0 and len(pools) == 0:
-LOG.debug('Adding distro default ntp pool servers')
pools = generate_server_names(cloud.distro.name)
+LOG.debug(
+'Adding distro default ntp pool servers: {}'.format(
+','.join(pools)))
params = {
'servers': servers,
diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
index d24f817..9ff1599 100644
--- a/tests/unittests/helpers.py
+++ b/tests/unittests/helpers.py
@@ -4,6 +4,7 @@ from __future__ import print_function
import functools
import json
+import logging
import os
import shutil
import sys
@@ -18,6 +19,10 @@ try:
from contextlib import ExitStack
except ImportError:
from contextlib2 import ExitStack
+try:
+from cStringIO import StringIO
+except ImportError:
+from io import StringIO
from cloudinit import helpers as ch
from cloudinit import util
@@ -87,6 +92,27 @@ class TestCase(unittest2.Test
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 225f898..257bb40 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -53,14 +53,12 @@ distros = ['centos', 'debian', 'fedora', 'opensuse',
> 'ubuntu']
>
>
> def handle(name, cfg, cloud, log, _args):
> -"""
> -Enable and configure ntp
> +"""Enable and configure ntp."""
>
> -ntp:
> - pools: ['0.{{distro}}.pool.ntp.org', '1.{{distro}}.pool.ntp.org']
> - servers: ['192.168.2.1']
> -
> -"""
> +if 'ntp' not in cfg:
Perform this check at the top and return before doing any other work. No need
to return True, nothing looks at the retval
> +LOG.debug(
> +"Skipping module named %s, not present or disabled by cfg", name)
> +return
>
> ntp_cfg = cfg.get('ntp', {})
>
> @@ -98,8 +90,10 @@ def install_ntp(install_func, packages=None,
> check_exe="ntpd"):
> install_func(packages)
>
>
> -def rename_ntp_conf(config=NTP_CONF):
We don't want to set default param values to module-level variables because
that breaks the ability to inject mocks in testing. Instead we check within the
function call if not config: config =
> +def rename_ntp_conf(config=None):
> """Rename any existing ntp.conf file and render from template"""
> +if config is None: # For testing
> +config = NTP_CONF
> if os.path.exists(config):
> util.rename(config, config + ".dist")
>
> @@ -117,8 +111,9 @@ def write_ntp_config_template(cfg, cloud):
> pools = cfg.get('pools', [])
>
> if len(servers) == 0 and len(pools) == 0:
> -LOG.debug('Adding distro default ntp pool servers')
> pools = generate_server_names(cloud.distro.name)
> +LOG.debug(
> +'Adding distro default ntp pool servers: %s', ','.join(pools))
Logs should have parameterized context to add value.
>
> params = {
> 'servers': servers,
> diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
> index d24f817..9ff1599 100644
> --- a/tests/unittests/helpers.py
> +++ b/tests/unittests/helpers.py
> @@ -87,6 +92,27 @@ class TestCase(unittest2.TestCase):
> class CiTestCase(TestCase):
> """This is the preferred test case base class unless user
> needs other test case classes below."""
> +
> +# Subclass overrides for specific test behavior
> +# Whether or not a unit test needs logfile setup
> +with_logs = False
Add a set of class attributes that allow us to conditionally setup common
resources in subclasses a unittest might need without having to create a
separate CiTestCase subclass definition.
> +
> +def setUp(self):
> +super(CiTestCase, self).setUp()
> +if self.with_logs:
> +# Create a log handler so unit tests can search expected logs.
> +logger = logging.getLogger()
> +self.logs = StringIO()
> +handler = logging.StreamHandler(self.logs)
> +self.old_handlers = logger.handlers
> +logger.handlers = [handler]
> +
> +def tearDown(self):
> +if self.with_logs:
> +# Remove the handler we setup
> +logging.getLogger().handlers = self.old_handlers
> +super(CiTestCase, self).tearDown()
> +
> def tmp_dir(self, dir=None, cleanup=True):
> # return a full path to a temporary directory that will be cleaned
> up.
> if dir is None:
> diff --git a/tests/unittests/test_handler/test_handler_ntp.py
> b/tests/unittests/test_handler/test_handler_ntp.py
> index 02bd304..aa6c8ae 100644
> --- a/tests/unittests/test_handler/test_handler_ntp.py
> +++ b/tests/unittests/test_handler/test_handler_ntp.py
> @@ -2,54 +2,31 @@
>
> from cloudinit.config import cc_ntp
> from cloudinit.sources import DataSourceNone
> -from cloudinit import templater
> from cloudinit import (distros, helpers, cloud, util)
> from ..helpers import FilesystemMockingTestCase, mock
>
> -import logging
> -import os
> -import shutil
> -import tempfile
>
> -LOG = logging.getLogger(__name__)
> +import os
>
> -NTP_TEMPLATE = """
> +NTP_TEMPLATE = b"""\
> ## template: jinja
> -
> -{% if pools %}# pools
Reduced this template to the bare minimum as unittests were really only
validating that the local template was rendered properly. No need to add
complexity in the jinja template as unit tests here aren't really testing the
actual template which is in ./templates/ntp.conf.*
> -{% endif %}
> -{% for pool in pools -%}
> -pool {{pool}} iburst
> -{% endfor %}
> -{%- if servers %}# servers
> -{% endif %}
> -{% for server in servers -%}
> -server {{server}} iburst
> -{% endfor %}
> -
> -"""
> -
> -
> -NTP_EXPECTED_UBUNTU = """
> -# pools
> -pool 0.mycompany.pool.ntp.org iburst
> -# servers
> -server 192.168.23.3 iburst
> -
> +servers {{servers}}
> +pools {{pools}}
> """
>
>
> class TestNtp(FilesystemMockingTestCase):
>
> +with_logs = T
Re: [Cloud-init-dev] [Merge] ~powersj/cloud-init:fix-blank-line into cloud-init:master
Review: Approve +1 land it -- https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/324490 Your team cloud-init commiters is requested to review the proposed merge of ~powersj/cloud-init:fix-blank-line into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 225f898..257bb40 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -69,18 +67,12 @@ def handle(name, cfg, cloud, log, _args):
> " but not a dictionary type,"
> " is a %s %instead"),
> type_utils.obj_name(ntp_cfg))
>
> -if 'ntp' not in cfg:
> -LOG.debug("Skipping module named %s,"
> - "not present or disabled by cfg", name)
> -return True
> -
> rename_ntp_conf()
> # ensure when ntp is installed it has a configuration file
> # to use instead of starting up with packaged defaults
> -write_ntp_config_template(ntp_cfg, cloud)
> +write_ntp_config_template(cfg['ntp'], cloud)
Sorry this leaked in from the follow up branch in which I flipped it back.
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324499
I'll pull this commit back in from the subsequent branch.
> install_ntp(cloud.distro.install_packages, packages=['ntp'],
> check_exe="ntpd")
> -
> # if ntp was already installed, it may not have started
> try:
> reload_ntp(systemd=cloud.distro.uses_systemd())
> diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
> index d24f817..9ff1599 100644
> --- a/tests/unittests/helpers.py
> +++ b/tests/unittests/helpers.py
> @@ -87,6 +92,27 @@ class TestCase(unittest2.TestCase):
> class CiTestCase(TestCase):
> """This is the preferred test case base class unless user
> needs other test case classes below."""
> +
> +# Subclass overrides for specific test behavior
> +# Whether or not a unit test needs logfile setup
> +with_logs = False
> +
> +def setUp(self):
> +super(CiTestCase, self).setUp()
> +if self.with_logs:
> +# Create a log handler so unit tests can search expected logs.
> +logger = logging.getLogger()
> +self.logs = StringIO()
> +handler = logging.StreamHandler(self.logs)
> +self.old_handlers = logger.handlers
> +logger.handlers = [handler]
I couldn't think of any reason not to have logs by default other than impact to
unit test runtime. I'll check how long unit tests take to run with and without
global log setup.
> +
> +def tearDown(self):
> +if self.with_logs:
> +# Remove the handler we setup
> +logging.getLogger().handlers = self.old_handlers
> +super(CiTestCase, self).tearDown()
> +
> def tmp_dir(self, dir=None, cleanup=True):
> # return a full path to a temporary directory that will be cleaned
> up.
> if dir is None:
> diff --git a/tests/unittests/test_handler/test_handler_ntp.py
> b/tests/unittests/test_handler/test_handler_ntp.py
> index 02bd304..aa6c8ae 100644
> --- a/tests/unittests/test_handler/test_handler_ntp.py
> +++ b/tests/unittests/test_handler/test_handler_ntp.py
> @@ -2,54 +2,31 @@
>
> from cloudinit.config import cc_ntp
> from cloudinit.sources import DataSourceNone
> -from cloudinit import templater
> from cloudinit import (distros, helpers, cloud, util)
> from ..helpers import FilesystemMockingTestCase, mock
>
> -import logging
> -import os
> -import shutil
> -import tempfile
>
> -LOG = logging.getLogger(__name__)
> +import os
>
> -NTP_TEMPLATE = """
> +NTP_TEMPLATE = b"""\
> ## template: jinja
> -
> -{% if pools %}# pools
Adding a test for the actual template now.
> -{% endif %}
> -{% for pool in pools -%}
> -pool {{pool}} iburst
> -{% endfor %}
> -{%- if servers %}# servers
> -{% endif %}
> -{% for server in servers -%}
> -server {{server}} iburst
> -{% endfor %}
> -
> -"""
> -
> -
> -NTP_EXPECTED_UBUNTU = """
> -# pools
> -pool 0.mycompany.pool.ntp.org iburst
> -# servers
> -server 192.168.23.3 iburst
> -
> +servers {{servers}}
> +pools {{pools}}
> """
>
>
> class TestNtp(FilesystemMockingTestCase):
>
> +with_logs = True
> +
> def setUp(self):
> super(TestNtp, self).setUp()
> self.subp = util.subp
> -self.new_root = tempfile.mkdtemp()
> -self.addCleanup(shutil.rmtree, self.new_root)
> +self.new_root = self.tmp_dir()
>
> def _get_cloud(self, distro, metadata=None):
> self.patchUtils(self.new_root)
> -paths = helpers.Paths({})
> +paths = helpers.Paths({'templates_dir': self.new_root})
> cls = distros.fetch(distro)
> mydist = cls(distro, {}, paths)
> myds = DataSourceNone.DataSourceNone({}, mydist, paths)
--
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
@scott Addressed your review comments thanks a lot. So unittest runtimes
1m30sec versus 2m20sec? Do we want to make the switch to have all CiTestCase
subclasses logged?
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 225f898..257bb40 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -53,14 +53,12 @@ distros = ['centos', 'debian', 'fedora', 'opensuse',
> 'ubuntu']
>
>
> def handle(name, cfg, cloud, log, _args):
> -"""
> -Enable and configure ntp
> +"""Enable and configure ntp."""
>
> -ntp:
> - pools: ['0.{{distro}}.pool.ntp.org', '1.{{distro}}.pool.ntp.org']
> - servers: ['192.168.2.1']
> -
> -"""
> +if 'ntp' not in cfg:
This is the same logic unchanged which was below, just hoisted up to the top of
the module. It would process {"ntp": {}} and install ntp. The ntp_cfg local
variable we create just below isn't actually referenced by the original check.
In this branch, and the followup cc-ntp-schema branch, we are allowing any
"ntp" key of any value in cfg. I understood from Ryan a couple days ago in IRC
that we actually want to allow for an empty ntp:configuration. The expected
behavior in this case would be to install ntp and leave it with the default 4
pool servers. This is also defined in the schema more strictly in the
cc-ntp-schema followup branch which declares "ntp" key as:
'ntp': {
'type': ['object', 'null'], # allow for a defined map or no
definition.
> +LOG.debug(
> +"Skipping module named %s, not present or disabled by cfg", name)
> +return
>
> ntp_cfg = cfg.get('ntp', {})
>
> @@ -69,18 +67,12 @@ def handle(name, cfg, cloud, log, _args):
> " but not a dictionary type,"
> " is a %s %instead"),
> type_utils.obj_name(ntp_cfg))
>
> -if 'ntp' not in cfg:
> -LOG.debug("Skipping module named %s,"
> - "not present or disabled by cfg", name)
> -return True
> -
> rename_ntp_conf()
> # ensure when ntp is installed it has a configuration file
> # to use instead of starting up with packaged defaults
> -write_ntp_config_template(ntp_cfg, cloud)
> +write_ntp_config_template(cfg['ntp'], cloud)
fixed.
> install_ntp(cloud.distro.install_packages, packages=['ntp'],
> check_exe="ntpd")
> -
> # if ntp was already installed, it may not have started
> try:
> reload_ntp(systemd=cloud.distro.uses_systemd())
> diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
> index d24f817..9ff1599 100644
> --- a/tests/unittests/helpers.py
> +++ b/tests/unittests/helpers.py
> @@ -87,6 +92,27 @@ class TestCase(unittest2.TestCase):
> class CiTestCase(TestCase):
> """This is the preferred test case base class unless user
> needs other test case classes below."""
> +
> +# Subclass overrides for specific test behavior
> +# Whether or not a unit test needs logfile setup
> +with_logs = False
> +
> +def setUp(self):
> +super(CiTestCase, self).setUp()
> +if self.with_logs:
> +# Create a log handler so unit tests can search expected logs.
> +logger = logging.getLogger()
> +self.logs = StringIO()
> +handler = logging.StreamHandler(self.logs)
> +self.old_handlers = logger.handlers
> +logger.handlers = [handler]
2m20 seconds on avg for tox vs 1m30 seconds to add logs to all CiTestCase
subclasses
> +
> +def tearDown(self):
> +if self.with_logs:
> +# Remove the handler we setup
> +logging.getLogger().handlers = self.old_handlers
> +super(CiTestCase, self).tearDown()
> +
> def tmp_dir(self, dir=None, cleanup=True):
> # return a full path to a temporary directory that will be cleaned
> up.
> if dir is None:
> diff --git a/tests/unittests/test_handler/test_handler_ntp.py
> b/tests/unittests/test_handler/test_handler_ntp.py
> index 02bd304..aa6c8ae 100644
> --- a/tests/unittests/test_handler/test_handler_ntp.py
> +++ b/tests/unittests/test_handler/test_handler_ntp.py
> @@ -59,294 +36,148 @@ class TestNtp(FilesystemMockingTestCase):
>
> @mock.patch("cloudinit.config.cc_ntp.util")
> def test_ntp_install(self, mock_util):
> -cc = self._get_cloud('ubuntu')
> -cc.distro = mock.MagicMock()
> -cc.distro.name = 'ubuntu'
> -mock_util.which.return_value = None
> +"""ntp_install installs via install_func when check_exe is absent."""
> +mock_util.which.return_value = None # check_exe not found.
> install_func = mock.MagicMock()
> -
> cc_ntp.install_ntp(install_func, packages=['ntpx'],
> check_exe='ntpdx')
>
> -self.assertTrue(install_func.called)
> mock_util.which.assert_called_with('ntpdx')
> -
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:cc-ntp-schema into
cloud-init:master with ~chad.smith/cloud-init:cc-ntp-testing as a prerequisite.
Commit message:
cc_ntp: Add schema definition and passive schema validation logging warnings.
cloud-config files are very flexible and permissive. This branch adds a
supported schema definition to cc_ntp so that we can properly inform users of
potential misconfiguration in their cloud-config yaml.
This first branch adds a jsonsschema definition to the cc_ntp module and
validation functions in cloudinit/config/schema which will log warnings about
invalid configuration values in the ntp section.
A cmdline tools/cloudconfig-schema is added which can be used in our dev
environments to quickly attempt to exercise the ntp schema. Eventually this
cloudconfig-schema tool will evolve into a cmdline tool we can provide to users
or a validation service which will allow folks to test their cloud-config files
without having to attempt deployment to find errors or inconsistencies.
LP: #1692916
Requested reviews:
cloud-init commiters (cloud-init-dev)
Related bugs:
Bug #1692916 in cloud-init: "Cloudinit modules should provide schema
validation to better alert consumers to unsupported config "
https://bugs.launchpad.net/cloud-init/+bug/1692916
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324499
cc_ntp: Add schema definition and passive schema validation logging warnings.
cloud-config files are very flexible and permissive. This branch adds a
supported schema definition to cc_ntp so that we can properly inform users of
potential misconfiguration in their cloud-config yaml.
This first branch adds a jsonsschema definition to the cc_ntp module and
validation functions in cloudinit/config/schema which will log warnings about
invalid configuration values in the ntp section.
A cmdline tools/cloudconfig-schema is added which can be used in our dev
environments to quickly attempt to exercise the ntp schema. Eventually this
cloudconfig-schema tool will evolve into a cmdline tool we can provide to users
or a validation service which will allow folks to test their cloud-config files
without having to attempt deployment to find errors or inconsistencies.
LP: #1692916
To test:
# download the attached schemas.tar
tar xf schemas.tar
./tools/cloudconfig-schema --doc # to dump rst from the schema definition
./tools/cloudconfig-schema --doc | rst2man | man -l - # manpage
for file in `ls schemas`; do echo '--- BEGIN ' $file;
cat schemas/$file; echo -e '--- OUTPUT: ';
./tools/cloudconfig-schema --config-file schemas/$file; echo -e
'--- END ' $file '\n\n'; done;
#run unit tests
tox -- --tests tests/unittests/test_handlers/test_handler_ntp.py
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master.
diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 5cc5453..a0da50f 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -36,6 +36,7 @@ servers or pools are provided, 4 pools will be used in the format
- 192.168.23.2
"""
+from cloudinit.config.schema import validate_cloudconfig_schema
from cloudinit import log as logging
from cloudinit.settings import PER_INSTANCE
from cloudinit import templater
@@ -52,21 +53,78 @@ NR_POOL_SERVERS = 4
distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
+# The schema definition for each cloud-config module is a strict contract for
+# describing supported configuration parameters for each cloud-config section.
+# It allows cloud-config to validate and alert users to invalid or ignored
+# configuration options before actually attempting to deploy with said
+# configuration.
+
+schema = {
+'id': 'cc_ntp',
+'name': 'NTP',
+'title': 'enable and configure ntp',
+'description': (
+'Handle ntp configuration. If ntp is not installed on the system and '
+'ntp configuration is specified, ntp will be installed. If there is a '
+'default ntp config file in the image or one is present in the '
+'distro\'s ntp package, it will be copied to ``/etc/ntp.conf.dist`` '
+'before any changes are made. A list of ntp pools and ntp servers can '
+'be provided under the ``ntp`` config key. If no ntp servers or pools '
+'are provided, 4 pools will be used in the format '
+'``{0-3}.{distro}.pool.ntp.org``.'),
+'distros': distros,
+'frequency': PER_INSTANCE,
+'type':
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve Diff comments: > diff --git a/tests/unittests/test_ds_identify.py > b/tests/unittests/test_ds_identify.py > index 9e14885..8559e1f 100644 > --- a/tests/unittests/test_ds_identify.py > +++ b/tests/unittests/test_ds_identify.py > @@ -210,6 +210,13 @@ class TestDsIdentify(CiTestCase): > mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n' > self._check_via_dict(mydata, rc=RC_FOUND, dslist=['NoCloud', > DS_NONE]) > > +def test_configured_list_with_none(self): > +"""If user set a datasource_list, that should be used.""" hah! > +mydata = copy.deepcopy(VALID_CFG['GCE']) > +cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg' > +mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n' > +self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE]) > + > > def blkid_out(disks=None): > """Convert a list of disk dictionaries into blkid content.""" -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
pre-approved on assumption of a new unit test for behavior, if the following
branch[1] lands before this one you could create a CiTestCase setting with_logs
= True and self.assertIn(" not warning", self.logs.getvalue())
References:
[1]
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/ds-identify-list-none-twice into cloud-init:master
Review: Approve +1 LGTM! -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324557 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/ds-identify-list-none-twice into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master has been updated. Description changed to: cc_ntp: Add schema definition and passive schema validation. cloud-config files are very flexible and permissive. This branch adds a supported schema definition to cc_ntp so that we can properly inform users of potential misconfiguration in their cloud-config yaml. This first branch adds a jsonsschema definition to the cc_ntp module and validation functions in cloudinit/config/schema which will log warnings about invalid configuration values in the ntp section. A cmdline tools/cloudconfig-schema is added which can be used in our dev environments to quickly attempt to exercise the ntp schema. Eventually this cloudconfig-schema tool will evolve into a cmdline tool we can provide to users or a validation service which will allow folks to test their cloud-config files without having to attempt deployment to find errors or inconsistencies. LP: #1692916 To test: # download the attached schemas.tar tar xf schemas.tar ./tools/cloudconfig-schema --doc # to dump rst from the schema definition ./tools/cloudconfig-schema --doc | rst2man | man -l - # manpage for file in `ls schemas`; do echo '--- BEGIN ' $file; cat schemas/$file; echo -e '--- OUTPUT: '; ./tools/cloudconfig-schema --config-file schemas/$file; echo -e '--- END ' $file '\n\n'; done; #run unit tests tox -- --tests tests/unittests/test_handlers/test_handler_ntp.py For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324499 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master has been updated. Commit Message changed to: cc_ntp: Add schema definition and passive schema validation. cloud-config files are very flexible and permissive. This branch adds a supported schema definition to cc_ntp so that we can properly inform users of potential misconfiguration in their cloud-config yaml. This first branch adds a jsonsschema definition to the cc_ntp module and validation functions in cloudinit/config/schema which will log warnings about invalid configuration values in the ntp section. A cmdline tools/cloudconfig-schema is added which can be used in our dev environments to quickly attempt to exercise the ntp schema. Eventually this cloudconfig-schema tool will evolve into a cmdline tool we can provide to users or a validation service which will allow folks to test their cloud-config files without having to attempt deployment to find errors or inconsistencies. LP: #1692916 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324499 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-schema into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~akaris/cloud-init:bug1684349 into cloud-init:master
Review: Needs Fixing Generally, if we are changing functionality, I'd love to see a unit test exercising this functionality so we don't have to test it with integration tests. It appears that this changeset will only pass this type problem down into ipv4mask2cidr and ipv6mask2cidr which will fall over with the same traceback as they attempt the same string search in mask(which is still an int). Scott mentioned in IRC today that he's working a branch cleaning up network_state so I'm wondering if we just wait on the cleanup there to see if it addresses this issue. -- https://code.launchpad.net/~akaris/cloud-init/+git/cloud-init/+merge/322992 Your team cloud-init commiters is requested to review the proposed merge of ~akaris/cloud-init:bug1684349 into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/fix-py26-tests-assertNone into cloud-init:master
Review: Approve
Wow good to know about format issues on cent6 as I've used it in other places.
I'll be wary for next time. +1
Diff comments:
> diff --git a/tests/unittests/test_handler/test_handler_ntp.py
> b/tests/unittests/test_handler/test_handler_ntp.py
> index 21f2ab1..bc4277b 100644
> --- a/tests/unittests/test_handler/test_handler_ntp.py
> +++ b/tests/unittests/test_handler/test_handler_ntp.py
> @@ -107,10 +107,10 @@ class TestNtp(FilesystemMockingTestCase):
> mycloud = self._get_cloud(distro)
> ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist
> # Create ntp.conf.tmpl which isn't read
> -with open('{}.tmpl'.format(ntp_conf), 'wb') as stream:
> +with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
wow, good to know for next time, as I've used format in my other branches.
> stream.write(b'NOT READ: ntp.conf..tmpl is primary')
> # Create ntp.conf.tmpl.
> -with open('{}.{}.tmpl'.format(ntp_conf, distro), 'wb') as stream:
> +with open('{0}.{1}.tmpl'.format(ntp_conf, distro), 'wb') as stream:
> stream.write(NTP_TEMPLATE)
> with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
> cc_ntp.write_ntp_config_template(cfg, mycloud)
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324585
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/fix-py26-tests-assertNone into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master has been updated. Commit Message changed to: cc_ntp: Add schema definition and passive schema validation. cloud-config files are very flexible and permissive. This branch adds a supported schema definition to cc_ntp so that we can properly inform users of potential misconfiguration in their cloud-config yaml. This first branch adds a jsonsschema definition to the cc_ntp module and validation functions in cloudinit/config/schema which will log warnings about invalid configuration values in the ntp section. A cmdline tools/cloudconfig-schema is added which can be used in our dev environments to quickly attempt to exercise the ntp schema. Eventually this cloudconfig-schema tool will evolve into a cmdline tool we can provide to users or a validation service which will allow folks to test their cloud-config files without having to attempt deployment to find errors or inconsistencies. LP: #1692916 For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324640 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:cc-ntp-schema-validation
into cloud-init:master.
Commit message:
cc_ntp: Add schema definition and passive schema validation.
cloud-config files are very flexible and permissive. This branch adds a
supported schema definition to cc_ntp so that we can properly inform users of
potential misconfiguration in their cloud-config yaml.
This first branch adds a jsonsschema definition to the cc_ntp module and
validation functions in cloudinit/config/schema which will log warnings about
invalid configuration values in the ntp section.
A cmdline tools/cloudconfig-schema is added which can be used in our dev
environments to quickly attempt to exercise the ntp schema. Eventually this
cloudconfig-schema tool will evolve into a cmdline tool we can provide to users
or a validation service which will allow folks to test their cloud-config files
without having to attempt deployment to find errors or inconsistencies.
LP: #1692916
Requested reviews:
Server Team CI bot (server-team-bot): continuous-integration
cloud-init commiters (cloud-init-dev)
Related bugs:
Bug #1692916 in cloud-init: "Cloudinit modules should provide schema
validation to better alert consumers to unsupported config "
https://bugs.launchpad.net/cloud-init/+bug/1692916
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324640
cc_ntp: Add schema definition and passive schema validation.
cloud-config files are very flexible and permissive. This branch adds a
supported schema definition to cc_ntp so that we can properly inform users of
potential misconfiguration in their cloud-config yaml.
This first branch adds a jsonsschema definition to the cc_ntp module and
validation functions in cloudinit/config/schema which will log warnings about
invalid configuration values in the ntp section.
A cmdline tools/cloudconfig-schema is added which can be used in our dev
environments to quickly attempt to exercise the ntp schema. Eventually this
cloudconfig-schema tool will evolve into a cmdline tool we can provide to users
or a validation service which will allow folks to test their cloud-config files
without having to attempt deployment to find errors or inconsistencies.
LP: #1692916
To test:
# download the attached schemas.tar
tar xf schemas.tar
./tools/cloudconfig-schema --doc # to dump rst from the schema definition
./tools/cloudconfig-schema --doc | rst2man | man -l - # manpage
for file in `ls schemas`; do echo '--- BEGIN ' $file;
cat schemas/$file; echo -e '--- OUTPUT: ';
./tools/cloudconfig-schema --config-file schemas/$file; echo -e
'--- END ' $file '\n\n'; done;
#run unit tests
tox -- --tests tests/unittests/test_handlers/test_handler_ntp.py
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master.
diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 5cc5453..573cd28 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -36,6 +36,7 @@ servers or pools are provided, 4 pools will be used in the format
- 192.168.23.2
"""
+from cloudinit.config.schema import validate_cloudconfig_schema
from cloudinit import log as logging
from cloudinit.settings import PER_INSTANCE
from cloudinit import templater
@@ -52,21 +53,78 @@ NR_POOL_SERVERS = 4
distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
+# The schema definition for each cloud-config module is a strict contract for
+# describing supported configuration parameters for each cloud-config section.
+# It allows cloud-config to validate and alert users to invalid or ignored
+# configuration options before actually attempting to deploy with said
+# configuration.
+
+schema = {
+'id': 'cc_ntp',
+'name': 'NTP',
+'title': 'enable and configure ntp',
+'description': (
+'Handle ntp configuration. If ntp is not installed on the system and '
+'ntp configuration is specified, ntp will be installed. If there is a '
+'default ntp config file in the image or one is present in the '
+'distro\'s ntp package, it will be copied to ``/etc/ntp.conf.dist`` '
+'before any changes are made. A list of ntp pools and ntp servers can '
+'be provided under the ``ntp`` config key. If no ntp servers or pools '
+'are provided, 4 pools will be used in the format '
+'``{0-3}.{distro}.pool.ntp.org``.'),
+'distros': distros,
+'frequency': PER_INSTANCE,
+'type':
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master has been updated. Description changed to: cc_ntp: Add schema definition and passive schema validation. cloud-config files are very flexible and permissive. This branch adds a supported schema definition to cc_ntp so that we can properly inform users of potential misconfiguration in their cloud-config yaml. This first branch adds a jsonsschema definition to the cc_ntp module and validation functions in cloudinit/config/schema which will log warnings about invalid configuration values in the ntp section. A cmdline tools/cloudconfig-schema is added which can be used in our dev environments to quickly attempt to exercise the ntp schema. Eventually this cloudconfig-schema tool will evolve into a cmdline tool we can provide to users or a validation service which will allow folks to test their cloud-config files without having to attempt deployment to find errors or inconsistencies. LP: #1692916 To test: # download the attached schemas.tar wget https://bugs.launchpad.net/cloud-init/+bug/1692916/+attachment/4883522/+files/schemas.tar tar xf schemas.tar ./tools/cloudconfig-schema --doc # to dump rst from the schema definition ./tools/cloudconfig-schema --doc | rst2man | man -l - # manpage for file in `ls schemas`; do echo '--- BEGIN ' $file; cat schemas/$file; echo -e '--- OUTPUT: '; ./tools/cloudconfig-schema --config-file schemas/$file; echo -e '--- END ' $file '\n\n'; done; #run unit tests tox -- --tests tests/unittests/test_handlers/test_handler_ntp.py For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324640 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1692093-sometimes-need-settle into cloud-init:master
> It doesn't look like you dropped blockdev as you say in your comment. The drop was a drop from the previous commit -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324639 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1692093-sometimes-need-settle into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master
Review comments addressed Scott. Thank you! I tweaked the rst doc layout a bit
to make sure we could use structured text within the Config keys doc section.
Let's talk about the runtime dependency concern tomorrow to see what we can do.
Not having the dependency defined means we'd have to remove the
validate_cloudconfig_schema from cc_ntp.handle() and we wouldn't actually
attempt to run passive validation on the user-defined cloud-config files so
users would not get warnings about potential issues in their cloud-config yaml.
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 5cc5453..573cd28 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -52,21 +53,78 @@ NR_POOL_SERVERS = 4
> distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
>
>
> +# The schema definition for each cloud-config module is a strict contract for
> +# describing supported configuration parameters for each cloud-config
> section.
> +# It allows cloud-config to validate and alert users to invalid or ignored
> +# configuration options before actually attempting to deploy with said
> +# configuration.
> +
> +schema = {
> +'id': 'cc_ntp',
> +'name': 'NTP',
> +'title': 'enable and configure ntp',
> +'description': (
> +'Handle ntp configuration. If ntp is not installed on the system and
> '
> +'ntp configuration is specified, ntp will be installed. If there is
> a '
> +'default ntp config file in the image or one is present in the '
> +'distro\'s ntp package, it will be copied to ``/etc/ntp.conf.dist`` '
> +'before any changes are made. A list of ntp pools and ntp servers
> can '
> +'be provided under the ``ntp`` config key. If no ntp servers or
> pools '
> +'are provided, 4 pools will be used in the format '
> +'``{0-3}.{distro}.pool.ntp.org``.'),
Good thought, dedented and stripped newlines when rendering structured text.
> +'distros': distros,
> +'frequency': PER_INSTANCE,
> +'type': 'object',
> +'properties': {
> +'ntp': {
> +'type': ['object', 'null'],
> +'properties': {
> +'pools': {
> +'type': 'array',
> +'items': {
> +'type': 'string',
> +'format': 'hostname'
> +},
> +'uniqueItems': True,
> +'description': (
> +'List of ntp pools. If both pools and servers are '
> +'empty, 4 default pool servers will be provided of '
> +'the format \{0-3\}.\{distro\}.pool.ntp.org.')
> +},
> +'servers': {
> +'type': 'array',
> +'items': {
> +'type': 'string',
> +'format': 'hostname'
> +},
> +'uniqueItems': True,
> +'description': (
> +'List of ntp servers. If both pools and servers are '
> +'empty, 4 default pool servers will be provided with
> '
> +'the format \{0-3\}.\{distro\}.pool.ntp.org.')
> +}
> +},
> +'required': [],
> +'additionalProperties': False
> +}
> +}
> +}
> +
> def handle(name, cfg, cloud, log, _args):
> """Enable and configure ntp."""
> -
> if 'ntp' not in cfg:
> LOG.debug(
> "Skipping module named %s, not present or disabled by cfg", name)
> return
> -
> ntp_cfg = cfg.get('ntp', {})
>
> +# TODO drop this when validate_cloudconfig_schema is strict=True
> if not isinstance(ntp_cfg, (dict)):
> raise RuntimeError(("'ntp' key existed in config,"
> " but not a dictionary type,"
> " is a %s %instead"),
> type_utils.obj_name(ntp_cfg))
>
> +validate_cloudconfig_schema(cfg, schema)
> rename_ntp_conf()
> # ensure when ntp is installed it has a configuration file
> # to use instead of starting up with packaged defaults
> diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py
> new file mode 100644
> index 000..9a732fd
> --- /dev/null
> +++ b/cloudinit/config/schema.py
> @@ -0,0 +1,141 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""schema.py: Set of module functions for processing cloud-config schema."""
> +
> +from cloudinit.util import read_file_or_url
> +import logging
> +from jsonschema import Draft4Validator, FormatChecker
Oops I had missed the jsonschema requirements.txt that I'd already added in a
previous branch. Sync'd the requirements.txt update here. Yes we'll have to
discuss what approach we can take here as we can't perform any validation if we
don't ha
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:cc-ntp-schema-validation into cloud-init:master
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 5cc5453..f2eb5a6 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -52,21 +54,79 @@ NR_POOL_SERVERS = 4
> distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
>
>
> +# The schema definition for each cloud-config module is a strict contract for
> +# describing supported configuration parameters for each cloud-config
> section.
> +# It allows cloud-config to validate and alert users to invalid or ignored
> +# configuration options before actually attempting to deploy with said
> +# configuration.
> +
> +schema = {
> +'id': 'cc_ntp',
> +'name': 'NTP',
> +'title': 'enable and configure ntp',
> +'description': dedent("""
corrected. thanks
> +Handle ntp configuration. If ntp is not installed on the system and
> +ntp configuration is specified, ntp will be installed. If there is a
> +default ntp config file in the image or one is present in the
> +distro's ntp package, it will be copied to ``/etc/ntp.conf.dist``
> +before any changes are made. A list of ntp pools and ntp servers can
> +be provided under the ``ntp`` config key. If no ntp ``servers`` or
> +``pools`` are provided, 4 pools will be used in the format
> +``{0-3}.{distro}.pool.ntp.org``."""),
> +'distros': distros,
> +'frequency': PER_INSTANCE,
> +'type': 'object',
> +'properties': {
> +'ntp': {
> +'type': ['object', 'null'],
> +'properties': {
> +'pools': {
> +'type': 'array',
> +'items': {
> +'type': 'string',
> +'format': 'hostname'
> +},
> +'uniqueItems': True,
> +'description': dedent("""
> +List of ntp pools. If both pools and servers are
> + empty, 4 default pool servers will be provided of
> + the format ``{0-3}.{distro}.pool.ntp.org``.""")
> +},
> +'servers': {
> +'type': 'array',
> +'items': {
> +'type': 'string',
> +'format': 'hostname'
> +},
> +'uniqueItems': True,
> +'description': dedent("""
> +List of ntp servers. If both pools and servers are
> + empty, 4 default pool servers will be provided with
> + the format ``{0-3}.{distro}.pool.ntp.org``.""")
> +}
> +},
> +'required': [],
> +'additionalProperties': False
> +}
> +}
> +}
> +
> +
> def handle(name, cfg, cloud, log, _args):
> """Enable and configure ntp."""
> -
> if 'ntp' not in cfg:
> LOG.debug(
> "Skipping module named %s, not present or disabled by cfg", name)
> return
> -
> ntp_cfg = cfg.get('ntp', {})
>
> +# TODO drop this when validate_cloudconfig_schema is strict=True
> if not isinstance(ntp_cfg, (dict)):
> raise RuntimeError(("'ntp' key existed in config,"
> " but not a dictionary type,"
> " is a %s %instead"),
> type_utils.obj_name(ntp_cfg))
>
> +validate_cloudconfig_schema(cfg, schema)
> rename_ntp_conf()
> # ensure when ntp is installed it has a configuration file
> # to use instead of starting up with packaged defaults
> diff --git a/tools/cloudconfig-schema b/tools/cloudconfig-schema
> new file mode 100755
> index 000..e26d671
> --- /dev/null
> +++ b/tools/cloudconfig-schema
> @@ -0,0 +1,62 @@
> +#!/usr/bin/env python3
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""cloudconfig-schema
> +
> +Validate existing files against cloud-config schema or provide supported
> schema
> +documentation.
> +"""
> +
> +import argparse
> +import json
> +from jsonschema.exceptions import ValidationError
> +import os
> +import sys
> +
> +
> +if 'avoid-pep8-E402-import-not-top-of-file':
> +_tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))
> +sys.path.insert(0, _tdir)
> +from cloudinit.config.schema import (
> +CLOUD_CONFIG_HEADER, SchemaValidationError, get_schema_doc,
> +get_schema, validate_cloudconfig_schema, validate_cloudconfig_file)
> +
> +
> +def error(message):
> +print(message, file=sys.stderr)
> +sys.exit(1)
> +
> +
> +def get_parser():
> +parser = argparse.ArgumentParser()
> +parser.add_argument('-c', '--config-file',
> +help='Path of the cloud-config yaml file to
> validate')
> +parser.add_argument('-d', '--doc', action="store_true", default=False,
> +help='Prin
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master
Review: Approve
Thanks for this good work approved pending your decision on the following
comments.
My comments will be made per the consolidated visual diff @
http://paste.ubuntu.com/24716726/
line 49 in the above diff:
Since order of datasources matters to determine which datasource is detectred
first in both cloudinit/settings.py CFG_BUILTIN and tools/ds-identify
DI_DSLIST_DEFAULT. Why are these two lists not ordered the same? AliYun is
placed just before Ec2 in ds-identify but way at the beginning settings.py.
Should we make an attempt to ensure both lists are consistently ordered?
line 95:
- Please add a comment in the code about the intent of NO_EC2_METADATA in in
cloudinit/sources/DataSourceEc2.py as you did in the commit message. The other
devs looking at the code for a subclass of DataSourceEc2 could know how and why
to make use of this value. commit messages are lost, comments live forever :).
line 103 cloudinit/sources/DataSourceAliYun.py:
- s/NO_EC2_MD/NO_EC2_METADATA/ let's reduce two letter acronyms throughout the
code as it frequently involves a second glance to figure out what the intent of
the variable is to determine context.
line 104 cloudinit/sources/DataSourceAliYun.py:
I see no unit test coverage of "elif self.cloud_platform ==
Platforms.NO_EC2_MD:"
line 124 tests/unittests/test_datasource/test_aliyun.py:
Since we have mocked _is_aliyun we aren't really testing anywhere that we
are getting this info from dmi. It might be better to mock util.read_dmi_data
as that is the interface we are calling into. Then we can validate the calls
into that interface as follows:
@mock.patch("cloudinit.util.read_dmi_data")
@httpretty.activate
def test_with_mock_server(self, m_read_dmi):
m_read_dmi.return_value = ay.ALIYUN_PRODUCT
self.regist_default_server()
self.ds.get_data()
self._test_get_data()
self._test_get_sshkey()
self._test_get_iid()
self._test_host_name()
m_read_dmi.assert_called_with('system-product-name')
line 140 tests/unittests/test_datasource/test_common.py
These network tests feel like they aren't all testing the right things. Why
is the test
Why is test_expected_nondefault_network_sources_found still passing or
needed? I was expecting to see that AliYun is dropped from nondefault sources.
line 175: Since DataSourceAliYum.ALIYUN_PRODUCT is already defined in the
module, we should use that variable in unit tests instead VALID_CFGs instead of
re-typing the string so there is only one source of truth for this data. It
also aids in grepping source to find out where ALIYUN_PRODUCT is used
throughout code and unit tests.
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324625
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:feature/enable-aliyun into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master
Thanks for the clarification and background on the datasource list ordering. And I see your point on git log (as that's how I determined what the intent of the var was in the first place). +1 on the separate mocked unit test for _is_aliyun() to exercise the interface we are actually calling. It'd be nice at some point if we had a fake dmidecode or SMBIOS test resource which could be used to seed datasource testing. Then we wouldn't really have to 'mock', just inject required dmi values and let things run. Per the importing the ay.ALIYUN_PRODUCT variable into unit tests, I agree with your consistency comment. Having a module variable imported into that module's unit tests only asserts that the module consistently references whatever the variable's value is (correct or incorrect). I'd suggest though that copying that string directly into the unit test does the same type of conisitency validation. The module writer could have defined a variable which won't work on aliyun systems in the first place, and then tested that ALIYUN_PRODUCT == "blah" as well. It doesn't actually assert that this value will work on aliyun environments. We'd probably have to rely on integration tests for that. Also this suggestion was to try to also bridge some consistency between our cloudinit/source/DataSourceAliyun.py and ds-identify. It feels like things are somewhat shallow in the consistency checks between tools/ds-identify logic and datasource detection logic in cloudinit/source/DataSource*py. It's probably due in part to the transition to strict ds validation etc. In a perfect world all this common logic to parse DMI data or metadata could be in one place, but I realize ds-identify has some constraints on where it is run (and speed) so loading up python modules probably doesn't make sense. -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324625 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master
Review: Approve +1 thanks for the discussion. -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324807 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:azure-di-id-asset-tag into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:azure-di-id-asset-tag
into cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324875
azure: ds-identify and Datasource.get_data checks DMI chassis-asset-tag
Azure sets a known chassis asset tag to 7783-7084-3265-9085-8269-3286-77. We
can inspect this in both ds-identify and DataSource.get_data to determine
whether we are on Azure. Added unit tests to cover these changes and some minor
tweaks to Exception error message content to give more context on malformed or
missing ovf-env.xml files.
LP: #1693939
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:azure-di-id-asset-tag into cloud-init:master.
diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index b9458ff..9dc9157 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -36,6 +36,8 @@ RESOURCE_DISK_PATH = '/dev/disk/cloud/azure_resource'
DEFAULT_PRIMARY_NIC = 'eth0'
LEASE_FILE = '/var/lib/dhcp/dhclient.eth0.leases'
DEFAULT_FS = 'ext4'
+# DMI chassis-asset-tag is set static for all azure instances
+AZURE_CHASSIS_ASSET_TAG = '7783-7084-3265-9085-8269-3286-77'
def find_storvscid_from_sysctl_pnpinfo(sysctl_out, deviceid):
@@ -319,7 +321,11 @@ class DataSourceAzureNet(sources.DataSource):
def get_data(self):
# azure removes/ejects the cdrom containing the ovf-env.xml
# file on reboot. So, in order to successfully reboot we
-# need to look in the datadir and consider that valid
+# need to look in the datadir and consider that valida
+asset_tag = util.read_dmi_data('chassis-asset-tag')
+if asset_tag != AZURE_CHASSIS_ASSET_TAG:
+LOG.info("Non-Azure DMI asset tag '%s' discovered.", asset_tag)
+return False
ddir = self.ds_cfg['data_dir']
candidates = [self.seed_dir]
@@ -694,7 +700,7 @@ def read_azure_ovf(contents):
try:
dom = minidom.parseString(contents)
except Exception as e:
-raise BrokenAzureDataSource("invalid xml: %s" % e)
+raise BrokenAzureDataSource("Invalid ovf-env.xml: %s" % e)
results = find_child(dom.documentElement,
lambda n: n.localName == "ProvisioningSection")
@@ -817,7 +823,8 @@ def load_azure_ds_dir(source_dir):
ovf_file = os.path.join(source_dir, "ovf-env.xml")
if not os.path.isfile(ovf_file):
-raise NonAzureDataSource("No ovf-env file found")
+raise NonAzureDataSource(
+"No ovf-env.xml file found at %s" % (source_dir))
with open(ovf_file, "rb") as fp:
contents = fp.read()
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 852ec70..f42d9c2 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -76,7 +76,9 @@ def construct_valid_ovf_env(data=None, pubkeys=None, userdata=None):
return content
-class TestAzureDataSource(TestCase):
+class TestAzureDataSource(CiTestCase):
+
+with_logs = True
def setUp(self):
super(TestAzureDataSource, self).setUp()
@@ -160,6 +162,12 @@ scbus-1 on xpt0 bus 0
self.instance_id = 'test-instance-id'
+def _dmi_mocks(key):
+if key == 'system-uuid':
+return self.instance_id
+elif key == 'chassis-asset-tag':
+return '7783-7084-3265-9085-8269-3286-77'
+
self.apply_patches([
(dsaz, 'list_possible_azure_ds_devs', dsdevs),
(dsaz, 'invoke_agent', _invoke_agent),
@@ -170,7 +178,7 @@ scbus-1 on xpt0 bus 0
(dsaz, 'set_hostname', mock.MagicMock()),
(dsaz, 'get_metadata_from_fabric', self.get_metadata_from_fabric),
(dsaz.util, 'read_dmi_data', mock.MagicMock(
-return_value=self.instance_id)),
+side_effect=_dmi_mocks)),
])
dsrc = dsaz.DataSourceAzureNet(
@@ -241,6 +249,23 @@ fdescfs/dev/fd fdescfs rw 0 0
res = get_path_dev_freebsd('/etc', mnt_list)
self.assertIsNotNone(res)
+@mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
+def test_non_azure_dmi_chassis_asset_tag(self, m_read_dmi_data):
+"""Report non-azure when DMI's chassis asset tag doesn't match.
+
+Return False when the asset tag doesn't match Azure's static
+AZURE_CH
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:cleanup/mask2cidr into cloud-init:master
looks much better thanks for this. minor inlines below
Diff comments:
> diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
> index db3c357..ef000ae 100644
> --- a/cloudinit/net/network_state.py
> +++ b/cloudinit/net/network_state.py
> @@ -692,6 +670,125 @@ class NetworkStateInterpreter(object):
> return subnets
>
>
> +def _normalize_subnet(subnet):
> +# Prune all keys with None values.
> +subnet = copy.deepcopy(subnet)
> +normal_subnet = dict((k, v) for k, v in subnet.items() if v)
> +
> +if subnet.get('type') == 'static':
> +normal_subnet.update(
> +_normalize_net_keys(normal_subnet, address_keys=('address',)))
> +normal_subnet['routes'] = [_normalize_route(r)
> + for r in subnet.get('routes', [])]
> +return normal_subnet
> +
> +
> +def _normalize_net_keys(network, address_keys=()):
> +
no need for extra lines.
> +"""Normalize dictionary network keys returning prefix and address keys.
> +
> +@param network: A dict of network-related definition containing prefix,
> +netmask and address_keys.
If we don't need netmask anymore in normalized dict, let's drop it here too.
> +@param address_keys: A tuple of keys to search for representing the
> address
> +or cidr. The first address_key discovered will be used for
> +normalization.
> +
> +@returns: A dict containing normalized prefix and matching addr_key.
> +"""
> +net = dict((k, v) for k, v in network.items() if v)
> +addr_key = None
> +for key in address_keys:
> +if net.get(key):
> +addr_key = key
> +break
> +if not addr_key:
> +message = (
> +'No config network address keys [%s] found in %s' %
> +(','.join(address_keys), network))
> +LOG.error(message)
> +raise ValueError(message)
> +
> +addr = net.get(addr_key)
> +ipv6 = is_ipv6_addr(addr)
> +netmask = net.get('netmask')
> +if "/" in addr:
> +toks = addr.split("/", 2)
> +net[addr_key] = toks[0]
> +# If prefix is defined by the user but addr_key is a CIDR, we
> +# silently overwrite the user's original prefix here. We should
> +# log a warning.
> +net['prefix'] = toks[1]
Agreed w/ Ryan here, can we log this warning?
> +try:
> +net['prefix'] = int(toks[1])
> +except ValueError:
> +# this supports input of /255.255.255.0
> +net['prefix'] = mask_to_net_prefix(toks[1])
> +
> +elif netmask:
> +net['prefix'] = mask_to_net_prefix(netmask)
> +else:
> +net['prefix'] = 64 if ipv6 else 24
> +
> +if ipv6:
> +# TODO: we could/maybe should add this back with the very uncommon
> +# 'netmask' for ipv6. We need a 'net_prefix_to_ipv6_mask' for that.
> +if 'netmask' in net:
> +del net['netmask']
> +else:
> +net['netmask'] = net_prefix_to_ipv4_mask(net['prefix'])
Ok I'm being dumb, why do we want netmask key around in the normalized
structure, don't we get all we want from our normalized prefix plus the
function net_prefix_to_ipv4_mask?
> +
> +prefix = net['prefix']
> +if prefix:
> +try:
> +net['prefix'] = int(prefix)
> +except ValueError:
> +raise TypeError(
> +'Network config prefix {} is not an integer'.format(prefix))
> +return net
> +
> +
> +def _normalize_route(route):
> +"""normalize a route.
> +return a dictionary with only:
> + 'type': 'route' (only present if it was present in input)
> + 'network': the network portion of the route as a string.
> + 'prefix': the network prefix for address as an integer.
> + 'metric': integer metric (only if present in input).
> + 'netmask': netmask (string) equivalent to prefix iff network is ipv4.
s/iff/if/
Also: If we don't need netmask anymore in normalized dict, let's drop it here
too.
> + """
> +# Prune None-value keys. Specifically allow 0 (a valid metric).
> +normal_route = dict((k, v) for k, v in route.items()
> +if v not in ("", None))
> +if 'destination' in normal_route:
> +normal_route['network'] = normal_route['destination']
> +del normal_route['destination']
> +
> +normal_route.update(
> +_normalize_net_keys(
> +normal_route, address_keys=('network', 'destination')))
> +
> +metric = normal_route.get('metric')
> +if metric:
> +try:
> +normal_route['metric'] = int(metric)
> +except ValueError:
> +raise TypeError(
> +'Route config metric {} is not an integer'.format(metric))
> +return normal_route
> +
> +
> +def _normalize_subnets(subnets):
> +if not subnets:
> +subnets = []
> +return [_normalize_subnet(s) for s in subnets]
> +
> +
> +def is_ipv6_addr(addr
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:azure-di-id-asset-tag into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceAzure.py
> b/cloudinit/sources/DataSourceAzure.py
> index b9458ff..9dc9157 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -319,7 +321,11 @@ class DataSourceAzureNet(sources.DataSource):
> def get_data(self):
> # azure removes/ejects the cdrom containing the ovf-env.xml
> # file on reboot. So, in order to successfully reboot we
> -# need to look in the datadir and consider that valid
> +# need to look in the datadir and consider that valida
> +asset_tag = util.read_dmi_data('chassis-asset-tag')
> +if asset_tag != AZURE_CHASSIS_ASSET_TAG:
> +LOG.info("Non-Azure DMI asset tag '%s' discovered.", asset_tag)
+1 on the noise, I do have a hard time following logs for cloud-init. Adding
cloud init logs to the sprint topics.
> +return False
> ddir = self.ds_cfg['data_dir']
>
> candidates = [self.seed_dir]
> @@ -817,7 +823,8 @@ def load_azure_ds_dir(source_dir):
> ovf_file = os.path.join(source_dir, "ovf-env.xml")
>
> if not os.path.isfile(ovf_file):
> -raise NonAzureDataSource("No ovf-env file found")
> +raise NonAzureDataSource(
> +"No ovf-env.xml file found at %s" % (source_dir))
On second glance we aren't really doing anything with that raise exception
message anyway. We just continue through a loop so it's probably not worth the
change in the first place. I'll back that out.
>
> with open(ovf_file, "rb") as fp:
> contents = fp.read()
> diff --git a/tests/unittests/test_datasource/test_azure.py
> b/tests/unittests/test_datasource/test_azure.py
> index 852ec70..f42d9c2 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -241,6 +249,23 @@ fdescfs/dev/fd fdescfs rw
>0 0
> res = get_path_dev_freebsd('/etc', mnt_list)
> self.assertIsNotNone(res)
>
> +@mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
> +def test_non_azure_dmi_chassis_asset_tag(self, m_read_dmi_data):
> +"""Report non-azure when DMI's chassis asset tag doesn't match.
> +
> +Return False when the asset tag doesn't match Azure's static
> +AZURE_CHASSIS_ASSET_TAG.
> +"""
> +# Return a non-matching asset tag value
> +nonazure_tag = dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'
> +m_read_dmi_data.return_value = nonazure_tag
> +dsrc = dsaz.DataSourceAzureNet(
> +{}, distro=None, paths=self.paths)
> +self.assertFalse(dsrc.get_data())
> +self.assertEqual(
I wanted this to be specific in this case to just show that we immediately
exited without calling other actions which happen to log content. Generally
yes, I'd agree we want assertIns for logs so we don't have to maintain these
unrelated tests if there are more logs added for in subsequent code runs. Since
this was the early exit point, I'd wanted it to fail if other logs were
generated.
> +"Non-Azure DMI asset tag '{0}'
> discovered.\n".format(nonazure_tag),
> +self.logs.getvalue())
> +
> def test_basic_seed_dir(self):
> odata = {'HostName': "myhost", 'UserName': "myuser"}
> data = {'ovfcontent': construct_valid_ovf_env(data=odata),
> @@ -696,6 +721,33 @@ class TestAzureBounce(TestCase):
> self.assertEqual(0, self.set_hostname.call_count)
>
>
> +class TestLoadAzureDsDir(CiTestCase):
> +"""Tests for load_azure_ds_dir."""
> +
> +def setUp(self):
> +self.source_dir = self.tmp_dir()
> +super(TestLoadAzureDsDir, self).setUp()
> +
> +def test_missing_ovf_env_xml_raises_non_azure_datasource_error(self):
> +"""load_azure_ds_dir raises an error When ovf-env.xml doesn't
> exit."""
> +with self.assertRaises(dsaz.NonAzureDataSource) as context_manager:
> +dsaz.load_azure_ds_dir(self.source_dir)
> +self.assertEqual(
> +'No ovf-env.xml file found at {0}'.format(self.source_dir),
> +str(context_manager.exception))
> +
> +def test_wb_invalid_ovf_env_xml_calls_read_azure_ovf(self):
> +"""load_azure_ds_dir calls read_azure_ovf to parse the xml."""
> +ovf_path = os.path.join(self.source_dir, 'ovf-env.xml')
> +with open(ovf_path, 'wb') as stream:
> +stream.write('invalid xml')
> +with self.assertRaises(dsaz.BrokenAzureDataSource) as
> context_manager:
> +dsaz.load_azure_ds_dir(self.source_dir)
> +self.assertEqual(
> +'Invalid ovf-env.xml: syntax error: line 1, column 0',
True, I was trying to assert that we are actually calling read_azure_ovf method
without mocks, and BrokenAzureDataSource is only raised fromt read_azure_ovf.
I marked the test as test_wb(whitebox) as it implies kno
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/net-convert-work-with-old into cloud-init:master
Review: Approve +1 on explicit kwargs for extensibility of the call for future revs as well as handling the existing param re-ordering difference. -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324948 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/net-convert-work-with-old into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:skip-jsonschema-unittest-when-missing-deps into cloud-init:master
Chad Smith has proposed merging
~chad.smith/cloud-init:skip-jsonschema-unittest-when-missing-deps into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325024
Tests: Skip jsonschema related unit tests when dependency is absent.
On some build environments we don't have python-jsonschema installed. Since
this dependency is an optional runtime dependency, we can also make it an
optional unit test dependency. Add a skip of related unittests when jsonschema
is not present.
Also, KeyError messages on CentOs don't have single quotes around the missing
'key-name'. Make our KeyError assertion a bit more flexible with the assertIn
call.
LP: #1695318
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:skip-jsonschema-unittest-when-missing-deps into
cloud-init:master.
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 6cafa63..25d7365 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -3,7 +3,7 @@
from cloudinit.config import cc_ntp
from cloudinit.sources import DataSourceNone
from cloudinit import (distros, helpers, cloud, util)
-from ..helpers import FilesystemMockingTestCase, mock
+from ..helpers import FilesystemMockingTestCase, mock, skipIf
import os
@@ -16,6 +16,12 @@ servers {{servers}}
pools {{pools}}
"""
+try:
+import jsonschema # NOQA
+_missing_jsonschema_dep = False
+except ImportError:
+_missing_jsonschema_dep = True
+
class TestNtp(FilesystemMockingTestCase):
@@ -232,6 +238,7 @@ class TestNtp(FilesystemMockingTestCase):
"servers []\npools {0}\n".format(default_pools),
content)
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
def test_ntp_handler_schema_validation_warns_non_string_item_type(self):
"""Ntp schema validation warns of non-strings in pools or servers.
@@ -252,6 +259,7 @@ class TestNtp(FilesystemMockingTestCase):
content = stream.read()
self.assertEqual("servers ['valid', None]\npools [123]\n", content)
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
def test_ntp_handler_schema_validation_warns_of_non_array_type(self):
"""Ntp schema validation warns of non-array pools or servers types.
@@ -272,6 +280,7 @@ class TestNtp(FilesystemMockingTestCase):
content = stream.read()
self.assertEqual("servers non-array\npools 123\n", content)
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
def test_ntp_handler_schema_validation_warns_invalid_key_present(self):
"""Ntp schema validation warns of invalid keys present in ntp config.
@@ -295,6 +304,7 @@ class TestNtp(FilesystemMockingTestCase):
"servers []\npools ['0.mycompany.pool.ntp.org']\n",
content)
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
def test_ntp_handler_schema_validation_warns_of_duplicates(self):
"""Ntp schema validation warns of duplicates in servers or pools.
diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py
index 3239e32..fd0e4f5 100644
--- a/tests/unittests/test_handler/test_schema.py
+++ b/tests/unittests/test_handler/test_schema.py
@@ -6,12 +6,18 @@ from cloudinit.config.schema import (
main)
from cloudinit.util import write_file
-from ..helpers import CiTestCase, mock
+from ..helpers import CiTestCase, mock, skipIf
from copy import copy
from six import StringIO
from textwrap import dedent
+try:
+import jsonschema # NOQA
+_missing_jsonschema_dep = False
+except ImportError:
+_missing_jsonschema_dep = True
+
class SchemaValidationErrorTest(CiTestCase):
"""Test validate_cloudconfig_schema"""
@@ -35,6 +41,7 @@ class ValidateCloudConfigSchemaTest(CiTestCase):
with_logs = True
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
def test_validateconfig_schema_non_strict_emits_warnings(self):
"""When strict is False validate_cloudconfig_schema emits warnings."""
schema = {'properties': {'p1': {'type': 'string'}}}
@@ -43,6 +50,7 @@ class ValidateCloudConfigSchemaTest(CiTestCase):
"Invalid config:\np1: -1 is not of type 'string'\n",
self.logs.getvalue())
+@skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
Re: [Cloud-init-dev] [Merge] lp:~harlowja/cloud-init/cmdline-kernel-logging into lp:~cloud-init-dev/cloud-init/trunk
Hello, Thank you for taking the time to contribute to cloud-init. Cloud-init has moved its revision control system to git. As a result, we are marking all bzr merge proposals as 'rejected'. If you would like to re-submit this proposal for review, please do so by following the current HACKING documentation at http://cloudinit.readthedocs.io/en/latest/topics/hacking.html. Also, it seems an evolution of this logging configuration has already landed in cloud-init master under the logcfg cloud-config module. If you feel current cloud-init changes don't meet your needs please either file a but against cloud-init or propose an iteration of this branch against master. https://cloudinit.readthedocs.io/en/latest/topics/logging.html -- https://code.launchpad.net/~harlowja/cloud-init/cmdline-kernel-logging/+merge/122755 Your team cloud-init commiters is requested to review the proposed merge of lp:~harlowja/cloud-init/cmdline-kernel-logging into lp:~cloud-init-dev/cloud-init/trunk. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] lp:~harlowja/cloud-init/cmdline-kernel-logging into lp:~cloud-init-dev/cloud-init/trunk
The proposal to merge lp:~harlowja/cloud-init/cmdline-kernel-logging into lp:~cloud-init-dev/cloud-init/trunk has been updated. Status: Needs review => Rejected For more details, see: https://code.launchpad.net/~harlowja/cloud-init/cmdline-kernel-logging/+merge/122755 -- Your team cloud-init commiters is requested to review the proposed merge of lp:~harlowja/cloud-init/cmdline-kernel-logging into lp:~cloud-init-dev/cloud-init/trunk. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] lp:~harlowja/cloud-init/query-tool-is-back into lp:~cloud-init-dev/cloud-init/trunk
The proposal to merge lp:~harlowja/cloud-init/query-tool-is-back into lp:~cloud-init-dev/cloud-init/trunk has been updated. Status: Needs review => Rejected For more details, see: https://code.launchpad.net/~harlowja/cloud-init/query-tool-is-back/+merge/123394 -- Your team cloud-init commiters is requested to review the proposed merge of lp:~harlowja/cloud-init/query-tool-is-back into lp:~cloud-init-dev/cloud-init/trunk. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] lp:~harlowja/cloud-init/query-tool-is-back into lp:~cloud-init-dev/cloud-init/trunk
Hello, Thank you for taking the time to contribute to cloud-init. Cloud-init has moved its revision control system to git. As a result, we are marking all bzr merge proposals as 'rejected'. If you would like to re-submit this proposal for review, please do so by following the current HACKING documentation at http://cloudinit.readthedocs.io/en/latest/topics/hacking.html. This branch will no longer apply against master, the cloud-init utility has moved to cloudinit/cmd/main.py and I think there are some review comments that would need to be addressed here to make this solution a bit more generic. -- https://code.launchpad.net/~harlowja/cloud-init/query-tool-is-back/+merge/123394 Your team cloud-init commiters is requested to review the proposed merge of lp:~harlowja/cloud-init/query-tool-is-back into lp:~cloud-init-dev/cloud-init/trunk. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:pyver-fix into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:pyver-fix into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325283
makefile: fix python 2/3 detection in the Makefile
Fixes CentOs make detection of python2 in a non-python3 environment
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:pyver-fix into cloud-init:master.
diff --git a/Makefile b/Makefile
index 09cd147..1cec9ae 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
CWD=$(shell pwd)
PYVER ?= $(shell for p in python3 python2; do \
- out=$(which $$p 2>&1) && echo $$p && exit; done; \
+ out=$$(command -v $$p 2>&1) && echo $$p && exit; done; \
exit 1)
noseopts ?= -v
@@ -53,6 +53,12 @@ unittest: clean_pyc
unittest3: clean_pyc
nosetests3 $(noseopts) tests/unittests
+ci-deps-ubuntu:
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro ubuntu --install
+
+ci-deps-centos:
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --install
+
pip-requirements:
@echo "Installing cloud-init dependencies..."
$(PIP_INSTALL) -r "$@.txt" -q
@@ -75,6 +81,7 @@ clean_pyc:
clean: clean_pyc
rm -rf /var/log/cloud-init.log /var/lib/cloud/
+
yaml:
@$(PYVER) $(CWD)/tools/validate-yaml.py $(YAML_FILES)
diff --git a/packages/bddeb b/packages/bddeb
index f415209..e45af6e 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -24,19 +24,6 @@ if "avoid-pep8-E402-import-not-top-of-file":
from cloudinit import templater
from cloudinit import util
-# Package names that will showup in requires which have unique package names.
-# Format is '': {'': , ...}.
-NONSTD_NAMED_PACKAGES = {
-'argparse': {'2': 'python-argparse', '3': None},
-'contextlib2': {'2': 'python-contextlib2', '3': None},
-'cheetah': {'2': 'python-cheetah', '3': None},
-'pyserial': {'2': 'python-serial', '3': 'python3-serial'},
-'pyyaml': {'2': 'python-yaml', '3': 'python3-yaml'},
-'six': {'2': 'python-six', '3': 'python3-six'},
-'pep8': {'2': 'pep8', '3': 'python3-pep8'},
-'pyflakes': {'2': 'pyflakes', '3': 'pyflakes'},
-}
-
DEBUILD_ARGS = ["-S", "-d"]
@@ -59,7 +46,6 @@ def write_debian_folder(root, templ_data, is_python2, cloud_util_deps):
else:
pyver = "3"
python = "python3"
-pkgfmt = "{}-{}"
deb_dir = util.abs_join(root, 'debian')
@@ -74,30 +60,23 @@ def write_debian_folder(root, templ_data, is_python2, cloud_util_deps):
params=templ_data)
# Write out the control file template
-reqs = run_helper('read-dependencies').splitlines()
+reqs_output = run_helper(
+'read-dependencies',
+args=['--distro', 'debian', '--python-version', pyver])
+reqs = reqs_output.splitlines()
test_reqs = run_helper(
-'read-dependencies', ['test-requirements.txt']).splitlines()
-
-pypi_pkgs = [p.lower().strip() for p in reqs]
-pypi_test_pkgs = [p.lower().strip() for p in test_reqs]
+'read-dependencies',
+['--requirements-file', 'test-requirements.txt',
+ '--system-pkg-names', '--python-version', pyver]).splitlines()
-# Map to known packages
requires = ['cloud-utils | cloud-guest-utils'] if cloud_util_deps else []
-test_requires = []
-lists = ((pypi_pkgs, requires), (pypi_test_pkgs, test_requires))
-for pypilist, target in lists:
-for p in pypilist:
-if p in NONSTD_NAMED_PACKAGES:
-if NONSTD_NAMED_PACKAGES[p][pyver]:
-target.append(NONSTD_NAMED_PACKAGES[p][pyver])
-else: # Then standard package prefix
-target.append(pkgfmt.format(python, p))
-
+# We consolidate all deps as Build-Depends as our package build runs all
+# tests so we need all runtime dependencies anyway.
+requires.extend(reqs + test_reqs + [python])
templater.render_to_file(util.abs_join(find_root(),
'packages', 'debian', 'control.in'),
util.abs_join(deb_dir, 'control'),
- params={'requires': ','.join(requires),
-
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:ci-deps into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342
pkg build ci: Add make ci-deps- target to automatically install pkgs
This change adds a couple of makefile targets for ci environments to install
all necessary dependencies for package builds and test runs.
It adds a number of arguments to ./tools/read-dependencies to facilitate
reading pip dependencies, translating pip deps to system package names and
optionally installing needed system-package dependencies on the local system.
This relocates all package dependency and translation logic into
./tools/read-dependencies instead of duplication found in packages/brpm and
packages/bddeb.
In this branch, we also define buildrequires as including all runtime requires
when rendering cloud-init.spec.in and debian/control files because our package
build infrastructure will also be running all unit test during the package
build process so we need runtime deps at build time.
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:ci-deps into cloud-init:master.
diff --git a/Makefile b/Makefile
index a3bfaf7..faa391c 100644
--- a/Makefile
+++ b/Makefile
@@ -53,6 +53,14 @@ unittest: clean_pyc
unittest3: clean_pyc
nosetests3 $(noseopts) tests/unittests
+ci-deps-ubuntu:
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro ubuntu --install
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro ubuntu --requirements-file test-requirements.txt --install
+
+ci-deps-centos:
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --install
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --requirements-file test-requirements.txt --install
+
pip-requirements:
@echo "Installing cloud-init dependencies..."
$(PIP_INSTALL) -r "$@.txt" -q
@@ -78,6 +86,7 @@ clean_pyc:
clean: clean_pyc
rm -rf /var/log/cloud-init.log /var/lib/cloud/
+
yaml:
@$(PYVER) $(CWD)/tools/validate-yaml.py $(YAML_FILES)
diff --git a/packages/bddeb b/packages/bddeb
index f415209..e45af6e 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -24,19 +24,6 @@ if "avoid-pep8-E402-import-not-top-of-file":
from cloudinit import templater
from cloudinit import util
-# Package names that will showup in requires which have unique package names.
-# Format is '': {'': , ...}.
-NONSTD_NAMED_PACKAGES = {
-'argparse': {'2': 'python-argparse', '3': None},
-'contextlib2': {'2': 'python-contextlib2', '3': None},
-'cheetah': {'2': 'python-cheetah', '3': None},
-'pyserial': {'2': 'python-serial', '3': 'python3-serial'},
-'pyyaml': {'2': 'python-yaml', '3': 'python3-yaml'},
-'six': {'2': 'python-six', '3': 'python3-six'},
-'pep8': {'2': 'pep8', '3': 'python3-pep8'},
-'pyflakes': {'2': 'pyflakes', '3': 'pyflakes'},
-}
-
DEBUILD_ARGS = ["-S", "-d"]
@@ -59,7 +46,6 @@ def write_debian_folder(root, templ_data, is_python2, cloud_util_deps):
else:
pyver = "3"
python = "python3"
-pkgfmt = "{}-{}"
deb_dir = util.abs_join(root, 'debian')
@@ -74,30 +60,23 @@ def write_debian_folder(root, templ_data, is_python2, cloud_util_deps):
params=templ_data)
# Write out the control file template
-reqs = run_helper('read-dependencies').splitlines()
+reqs_output = run_helper(
+'read-dependencies',
+args=['--distro', 'debian', '--python-version', pyver])
+reqs = reqs_output.splitlines()
test_reqs = run_helper(
-'read-dependencies', ['test-requirements.txt']).splitlines()
-
-pypi_pkgs = [p.lower().strip() for p in reqs]
-pypi_test_pkgs = [p.lower().strip() for p in test_reqs]
+'read-dependencies',
+['--requirements-file', 'test-requirements.txt',
+ '--system-pkg-names', '--python-version', pyver]).splitlines()
-# Map to known packages
requires = ['cloud-utils | cloud-guest-utils'] if cloud_util_deps else []
-test_requires = []
-lists = ((pypi_pkgs, requires), (pypi_test_pkgs, test_requires))
-for pypilist, target in lists:
-for p in pypilist:
-if p in NONSTD_NAMED_PACKAGES:
-if NONSTD_NAMED_PACKAGES[p][pyver]:
-
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Commit Message changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Commit Message changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git git clone cd cloud-init make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Commit Message changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git python-argparse git clone -b ci-deps https://git.launchpad.net/~chad.smith/cloud-init cd cloud-init make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Commit Message changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Description changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git python-argparse git clone -b ci-deps https://git.launchpad.net/~chad.smith/cloud-init cd cloud-init make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Description changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git python-argparse git clone -b master https://git.launchpad.net/cloud-init cd cloud-init git remote add chad.smith https://git.launchpad.net/~chad.smith/cloud-init git fetch chad.smith git merge chad.smith/ci-deps make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Commit Message changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. Additionally, this branch converts packages/(redhat|suse)/cloud-init.spec.in from cheetah templates to jinja to allow building python3 envs. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Description changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git python-argparse make git clone -b master https://git.launchpad.net/cloud-init cd cloud-init git remote add chad.smith https://git.launchpad.net/~chad.smith/cloud-init git fetch chad.smith git merge chad.smith/ci-deps make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
Addressed review comments plus:
- dropped argparse filtering from read-dependencies and brpm now that master
has baked argparse direclty into the spec for py26
- added dry-run param to read-dependencies to allow for viewing intended
install actions
- made sure Requires in redhat and suse specs no longer contained any
dependencies from test-requirements.txt
Diff comments:
> diff --git a/Makefile b/Makefile
> index a3bfaf7..faa391c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -78,6 +86,7 @@ clean_pyc:
> clean: clean_pyc
> rm -rf /var/log/cloud-init.log /var/lib/cloud/
>
> +
accidental, dropped.
> yaml:
> @$(PYVER) $(CWD)/tools/validate-yaml.py $(YAML_FILES)
>
> diff --git a/tools/read-dependencies b/tools/read-dependencies
> index f434905..149c2b0 100755
> --- a/tools/read-dependencies
> +++ b/tools/read-dependencies
> @@ -1,43 +1,194 @@
> #!/usr/bin/env python
> +"""List pip dependencies or system package dependencies for cloud-init."""
>
> # You might be tempted to rewrite this as a shell script, but you
> # would be surprised to discover that things like 'egrep' or 'sed' may
> # differ between Linux and *BSD.
>
> +try:
> +from argparse import ArgumentParser
> +except ImportError:
> +raise RuntimeError(
> +'Could not import python-argparse. Please install python-argparse '
> +'package to continue')
> +
> +import json
> import os
> import re
> -import sys
> import subprocess
> +import sys
>
> -if 'CLOUD_INIT_TOP_D' in os.environ:
> -topd = os.path.realpath(os.environ.get('CLOUD_INIT_TOP_D'))
> -else:
> -topd = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
>
> -for fname in ("setup.py", "requirements.txt"):
> -if not os.path.isfile(os.path.join(topd, fname)):
> -sys.stderr.write("Unable to locate '%s' file that should "
> - "exist in cloud-init root directory." % fname)
> -sys.exit(1)
> -
> -if len(sys.argv) > 1:
> -reqfile = sys.argv[1]
> -else:
> -reqfile = "requirements.txt"
> -
> -with open(os.path.join(topd, reqfile), "r") as fp:
> -for line in fp:
> -line = line.strip()
> -if not line or line.startswith("#"):
> +# Map the appropriate package dir needed for each distro choice
> +DISTRO_PKG_TYPE_MAP = {
> +'centos': 'redhat',
> +'redhat': 'redhat',
> +'debian': 'debian',
> +'ubuntu': 'debian',
> +'opensuse': 'suse',
> +'suse': 'suse'
> +}
> +
> +DISTRO_INSTALL_PKG_CMD = {
> +'centos': ['yum', 'install', '--assumeyes'],
> +'redhat': ['yum', 'install', '--assumeyes'],
> +'debian': ['apt', 'install', '-y'],
> +'ubuntu': ['apt', 'install', '-y'],
> +'opensuse': ['zypper', 'install'],
> +'suse': ['zypper', 'install']
> +}
> +
> +PY_26_ONLY = ['argparse']
> +
> +# List of base system packages required to start using make
> +EXTRA_SYSTEM_BASE_PKGS = ['make', 'sudo', 'tar']
> +
> +
> +# JSON definition of distro-specific package dependencies
> +DISTRO_PKG_DEPS_PATH = "packages/pkg-deps.json"
> +
> +
> +def get_parser():
> +"""Return an argument parser for this command."""
> +parser = ArgumentParser(description=__doc__)
> +parser.add_argument(
> +'-r', '--requirements-file', type=str, dest='req_file',
> +default='requirements.txt', help='The pip-style requirements file')
> +parser.add_argument(
> +'-d', '--distro', type=str, choices=DISTRO_PKG_TYPE_MAP.keys(),
> +help='The name of the distro to generate package deps for.')
> +parser.add_argument(
> +'-s', '--system-pkg-names', action='store_true', default=False,
> +dest='system_pkg_names',
> +help='The name of the distro to generate package deps for.')
> +parser.add_argument(
> +'-i', '--install', action='store_true', default=False,
> +dest='install',
> +help='When specified, install the required system packages.')
> +parser.add_argument(
> +'-v', '--python-version', type=str, dest='python_version',
> default="2",
> +choices=["2", "3"],
> +help='The version of python we want to generate system package '
> + 'dependencies for.')
> +return parser
> +
> +
> +def get_package_deps_from_json(topdir, distro):
> +"""Get a dict of build and runtime package requirements for a distro.
> +
> +@param topdir: The root directory in which to search for the
> +DISTRO_PKG_DEPS_PATH json blob of package requirements information.
> +@param distro: The specific distribution shortname to pull dependencies
> +for.
> +@return: Dict containing "requires", "build-requires" and "rename" lists
> +for a given distribution.
> +"""
> +with open(os.path.join(topdir, DISTRO_PKG_DEPS_PATH), 'r') as stream:
> +deps = json.loads(stream.read())
> +if distro is None:
> +return {}
> +return deps[DISTRO_PKG_TYPE_MAP[distro]]
> +
> +
> +def parse_pip_requirements(requirements_
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Description changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git python34 make git clone -b master https://git.launchpad.net/cloud-init cd cloud-init git remote add chad.smith https://git.launchpad.net/~chad.smith/cloud-init git fetch chad.smith git merge chad.smith/ci-deps make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps into cloud-init:master has been updated. Description changed to: pkg build ci: Add make ci-deps- target to automatically install pkgs This change adds a couple of makefile targets for ci environments to install all necessary dependencies for package builds and test runs. It adds a number of arguments to ./tools/read-dependencies to facilitate reading pip dependencies, translating pip deps to system package names and optionally installing needed system-package dependencies on the local system. This relocates all package dependency and translation logic into ./tools/read-dependencies instead of duplication found in packages/brpm and packages/bddeb. In this branch, we also define buildrequires as including all runtime requires when rendering cloud-init.spec.in and debian/control files because our package build infrastructure will also be running all unit test during the package build process so we need runtime deps at build time. To test: lxc launch images:centos/6 lxc exec bash yum install git make python-argparse git clone -b master https://git.launchpad.net/cloud-init cd cloud-init git fetch --tags git remote add chad.smith https://git.launchpad.net/~chad.smith/cloud-init git fetch chad.smith git merge chad.smith/ci-deps make ci-deps-centos make rpm For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
Comments addressed, thank you much -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:schema-autodoc into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:schema-autodoc into cloud-init:master has been updated. Commit Message changed to: docs: Automatically generate module docs form schema attribute if present We have started adding jsonschema defintions for cloudconfig modules (cc_ntp). This branch allows us render sphinx docs using the module's shema definition instead of using the module's docstring. It allows us to generate our module documentation directly from the schema definition and avoid duplicating that documentation in the module-level docstring. The corresponding module documentation is extended a bit to differentiate between config schema and potential examples. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325507 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:schema-autodoc into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:schema-autodoc into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:schema-autodoc into
cloud-init:master.
Commit message:
docs: Automatically generate module docs form schema attribute if present
We have started adding jsonschema defintions for cloudconfig modules (cc_ntp).
This branch allows us render sphinx docs using the module's shema definition
instead of using the module's docstring.
It allows us to generate our module documentation directly from the schema
definition and avoid duplicating that documentation in the module-level
docstring. The corresponding module documentation is extended a bit to
differentiate between config schema and potential examples.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325507
docs: Automatically generate module docs form schema attribute if present
We have started adding jsonschema defintions for cloudconfig modules (cc_ntp).
This branch allows us render sphinx docs using the module's shema definition
instead of using the module's docstring.
It allows us to generate our module documentation directly from the schema
definition and avoid duplicating that documentation in the module-level
docstring. The corresponding module documentation is extended a bit to
differentiate between config schema and potential examples.
To test:
tox -e docs
xdg-open doc/rtd_html/topics/modules.html # look over the cc_ntp module
# merge this branch and remove part of the module docstring
cloudinit/config/cc_ntp.py
xdg-open doc/rtd_html/topics/modules.html # to view the differences in
styling and content of cc_ntp
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:schema-autodoc into cloud-init:master.
diff --git a/doc/rtd/conf.py b/doc/rtd/conf.py
index 66b3b65..0ea3b6b 100644
--- a/doc/rtd/conf.py
+++ b/doc/rtd/conf.py
@@ -10,6 +10,7 @@ sys.path.insert(0, os.path.abspath('./'))
sys.path.insert(0, os.path.abspath('.'))
from cloudinit import version
+from cloudinit.config.schema import get_schema_doc
# Supress warnings for docs that aren't used yet
# unused_docs = [
@@ -75,3 +76,12 @@ html_theme_options = {
# The name of an image file (relative to this directory) to place at the top
# of the sidebar.
html_logo = 'static/logo.png'
+
+def generate_docstring_from_schema(app, what, name, obj, options, lines):
+"""Override module docs from schema when present."""
+if what == 'module' and hasattr(obj, "schema"):
+del lines[:]
+lines.extend(get_schema_doc(obj.schema).split('\n'))
+
+def setup(app):
+app.connect('autodoc-process-docstring', generate_docstring_from_schema)
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
@scott I think I've addressed all comments here
Diff comments:
> diff --git a/packages/pkg-deps.json b/packages/pkg-deps.json
> new file mode 100644
> index 000..2100ec7
> --- /dev/null
> +++ b/packages/pkg-deps.json
> @@ -0,0 +1,84 @@
> +{
> + "debian" : {
> + "build-requires" : [
> + "debhelper",
> + "dh-python",
> + "dh-systemd"
> + ],
> + "renames" : {
> + "pep8" : {
> +"2" : "pep8",
dropped as we are only looking to support xenial and greater currently for pkg
builds etc. We can deal with this if we have to SRU to trusty for a customer
> +"3" : "python3-pep8"
> + },
> + "pyflakes" : {
Good points, since master is really only generally targeting (via SRUs) xenial
and above I've dropped some renames that don't matter anymore. We can add more
distro-specific pkg renames in if end up having to pull a giant SRU into trusty.
> +"2" : "pyflakes",
> +"3" : "pyflakes"
> + },
> + "pyyaml" : {
> +"2" : "python-yaml",
> +"3" : "python3-yaml"
> + },
> + "six" : {
> +"2" : "python-six",
yep removed.
> +"3" : "python3-six"
> + },
> + "contextlib2" : {
> +"2" : "python-contextlib2",
> +"3" : null
> + },
> + "pyserial" : {
> +"2" : "python-serial",
> +"3" : "python3-serial"
> + }
> + },
> + "requires" : [
> + "procps"
> + ]
> + },
> + "redhat" : {
> + "build-requires" : [
> + "python-devel",
> + "python-setuptools"
> + ],
> + "renames" : {
> + "pyyaml" : {
> +"2" : "PyYAML",
null means use default.
> +"3" : null
> + },
> + "pyserial" : {
> +"2" : "pyserial",
> +"3" : null
> + }
Yeah strange I can't find pyserial either on centos, though I see references to
it at http://pyserial.readthedocs.io/en/latest/pyserial.html. We also have the
expectation of that missing deps within a try/except ImportError block so we
are good not documenting it.
> + },
> + "requires" : [
> + "e2fsprogs",
> + "iproute",
> + "net-tools",
> + "procps",
> + "rsyslog",
> + "shadow-utils",
> + "sudo >= 1.7.2p2-3"
> + ]
> + },
> + "suse" : {
> + "renames" : {
> + "pyyaml" : {
> +"2" : "python-yaml",
> +"3" : null
> + }
> + },
> + "build-requires" : [
> + "fdupes",
> + "filesystem",
> + "python-devel",
> + "python-setuptools"
> + ],
> + "requires" : [
> + "iproute2",
> + "e2fsprogs",
> + "net-tools",
> + "procps",
> + "sudo"
> + ]
> + }
> +}
--
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:ci-deps into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps into cloud-init:master
Addressed renames for six, pyyaml an pyflakes to use python34-* where needed. I'd like to wait for a separate branch to sort our python3* issues on CentOS to keep this branch a bit more focused as a "best-effort" for python34 there. Our existing ci can use this current branch for python2 CentOS package builds, so we can drop our current dependency/setup duplication. As you suggested, there are likely a couple of bugs we'll need to sort when we attempt full py3 package support on CentOS. When we tackle python3 support in earnest on CentOS, we may also have to add the facility to blacklist/ignore certain optional runtime requirements which we find don't exist for python3 in CentOS (like pyserial, jsonschema etc.). -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325342 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:schema-autodoc into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:schema-autodoc into cloud-init:master has been updated. Commit Message changed to: docs: Automatically generate module docs form schema attribute if present We have started adding jsonschema definitions for cloudconfig modules (cc_ntp). This branch allows us render sphinx docs using the module's shema definition instead of using the module's docstring. This allows us to avoid duplicating schema documentation in the module-level docstring and schema definition. The corresponding module documentation is extended a bit to differentiate between config schema and potential examples. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325507 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:schema-autodoc into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:schema-autodoc into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:schema-autodoc into cloud-init:master has been updated. Description changed to: docs: Automatically generate module docs form schema attribute if present We have started adding jsonschema defintions for cloudconfig modules (cc_ntp). This branch allows us render sphinx docs using the module's shema definition instead of using the module's docstring. It allows us to generate our module documentation directly from the schema definition and avoid duplicating that documentation in the module-level docstring. The corresponding module documentation is extended a bit to differentiate between config schema and potential examples. To test: tox -e docs xdg-open doc/rtd_html/topics/modules.html # look over the cc_ntp module # merge this branch and remove part of the module docstring cloudinit/config/cc_ntp.py tox -e docs xdg-open doc/rtd_html/topics/modules.html # to view the differences in styling and content of cc_ntp For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325507 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:schema-autodoc into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/fix-ci-redhat into cloud-init:master
Diff comments:
> diff --git a/tools/read-dependencies b/tools/read-dependencies
> index 4ba2c1b..f22f4a5 100755
> --- a/tools/read-dependencies
> +++ b/tools/read-dependencies
> @@ -144,12 +146,18 @@ def main(distro):
> topd = os.path.realpath(os.environ.get('CLOUD_INIT_TOP_D'))
> else:
> topd = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
> -req_path = os.path.join(topd, args.req_file)
> -if not os.path.isfile(req_path):
> -sys.stderr.write("Unable to locate '%s' file that should "
> - "exist in cloud-init root directory." % req_path)
> -return 1
> -pip_pkg_names = parse_pip_requirements(req_path)
> +
> +if args.req_files is None:
> +args.req_files = [os.path.join(topd, DEFAULT_REQUIREMENTS)]
this falls over if someone specifies the full path to the file. But it's a
simple tool so I think we are fine dealing w/ that (as we'll error out just
below).
> +if not os.path.isfile(args.req_files[0]):
Since we support multiple requirenments files now any objection to the
following?
--- a/tools/read-dependencies
+++ b/tools/read-dependencies
@@ -149,11 +149,14 @@ def main(distro):
if args.req_files is None:
args.req_files = [os.path.join(topd, DEFAULT_REQUIREMENTS)]
-if not os.path.isfile(args.req_files[0]):
-sys.stderr.write("Unable to locate '%s' file that should "
- "exist in cloud-init root directory." %
- args.req_files[0])
-sys.exit(1)
+for req_file in args.req_files:
+bad_files = [
+req_file for req_file in args.req_files
+if not os.path.isfile(req_file)]
+if bad_files:
+sys.stderr.write(
+"Unable to find requirements files: %s\n" % ','.join(bad_files))
+sys.exit(1)
pip_pkg_names = set()
for req_path in args.req_files:
> +sys.stderr.write("Unable to locate '%s' file that should "
> + "exist in cloud-init root directory." %
> + args.req_files[0])
> +sys.exit(1)
> +
> +pip_pkg_names = set()
> +for req_path in args.req_files:
> +pip_pkg_names.update(set(parse_pip_requirements(req_path)))
> deps_from_json = get_package_deps_from_json(topd, args.distro)
> renames = deps_from_json.get('renames', {})
> translated_pip_names = translate_pip_to_system_pkg(
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325679
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/fix-ci-redhat into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/fix-ci-redhat into cloud-init:master
Diff comments:
> diff --git a/tools/run-centos b/tools/run-centos
> index de21d75..f812e93 100755
> --- a/tools/run-centos
> +++ b/tools/run-centos
> @@ -63,26 +78,53 @@ inside() {
> }
>
> inject_cloud_init(){
> -local name="$1"
> -tarball_name='cloud-init.tar.gz'
> -top_d=$(git rev-parse --show-toplevel) ||
> -fail "failed to get top level"
> -cd "$top_d" ||
> -fail "failed to cd to git top dir"
> -tar_folder=${PWD##*/}
> -cd ..
> -tar -czf "$TEMP_D/$tarball_name" "$tar_folder" ||
> -fail "failed: creating tarball_name"
> -cd "$tar_folder" ||
> -fail "failed: changing directory"
> -
> -user='centos'
> -tarball="/home/$user/$tarball_name"
> -inside "$name" useradd "$user"
> -lxc file push "$TEMP_D/$tarball_name" "$name/home/$user"/
> -inside "$name" chown "$user:$user" "$tarball"
> -inside_as "$name" "$user" tar -C "/home/$user" -xzf "$tarball" ||
> -fail "failed: extracting tarball"
> +# take current cloud-init git dir and put it inside $name at
> +# ~$user/cloud-init.
> +local name="$1" user="$2" top_d="" dname="" pstat=""
> +top_d=$(git rev-parse --show-toplevel) || {
> +errorrc "Failed to get git top level in $PWD";
> +return
> +}
> +dname=$(basename "${top_d}") || return
> +debug 1 "collecting ${top_d} ($dname) into user $user in $name."
> +tar -C "${top_d}/.." -cpf - "$dname" |
> +inside_as "$name" "$user" sh -ec '
> +dname=$1
> +rm -Rf "$dname"
> +tar -xpf -
> +[ "$dname" = "cloud-init" ] || mv "$dname" cloud-init' \
> +extract "$dname"
> +[ "${PIPESTATUS[*]}" = "0 0" ] || {
> +error "Failed to push tarball of '$top_d' into $name" \
> +" for user $user (dname=$dname)"
> +return 1
> +}
> +return 0
> +}
> +
> +prep() {
> +# we need some very basic things not present in the container.
> +# - git
> +# - tar (CentOS 6 lxc container does not have it)
> +# - python-argparse (or python3)
> +local needed=""
> +needed=""
> +for pair in tar:tar git:git; do
I guess you are future-proofing here with the pairs declaration? Since tar and
git's pkg and cmd are the same do we need to define : in the loop?
> +pkg=${pair#*:}
> +cmd=${pair%%:*}
> +command -v $cmd >/dev/null 2>&1 || needed="${needed} $pkg"
> +done
> +if ! command -v python3; then
> +python -c "import argparse" >/dev/null 2>&1 ||
> +needed="${needed} python-argparse"
> +fi
> +needed=${needed# }
> +if [ -z "$needed" ]; then
> +error "No prep packages needed"
> +return 0
> +fi
> +error "Installing prep packages: ${needed}"
> +yum install --assumeyes ${needed}
> }
>
> start_container() {
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325679
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/fix-ci-redhat into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps-fixes into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:ci-deps-fixes into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325693
ci dependency installs: Add --test-distro param to read-dependencies to install
deps
read-dependencies now takes --test-distro param to indicate we want to install
all system package depenencies to allow for testing and building for our
continous integration environment. It allows us to install all needed deps on a
fresh system with python3 ./tools/read-depenencies --distro ubuntu
--test-distro [--dry-run].
Additionally read-dependencies now looks at what version of python is running
the script (py2 vs p3) and opts to install python 2 or 3 system deps
respectively. This behavior can still be overridden with python3
./tools/read-dependencies ... --python-version 2.
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:ci-deps-fixes into cloud-init:master.
diff --git a/Makefile b/Makefile
index c752530..3a69d87 100644
--- a/Makefile
+++ b/Makefile
@@ -54,12 +54,10 @@ unittest3: clean_pyc
nosetests3 $(noseopts) tests/unittests
ci-deps-ubuntu:
- @$(PYVER) $(CWD)/tools/read-dependencies --distro ubuntu --install --python-version 3
- @$(PYVER) $(CWD)/tools/read-dependencies --distro ubuntu --requirements-file test-requirements.txt --install --python-version 3
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro-ubuntu --ci-deps
ci-deps-centos:
- @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --install
- @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --requirements-file test-requirements.txt --install
+ @$(PYVER) $(CWD)/tools/read-dependencies --distro centos --ci-deps
pip-requirements:
@echo "Installing cloud-init dependencies..."
diff --git a/packages/bddeb b/packages/bddeb
index e45af6e..c7172d0 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -72,7 +72,9 @@ def write_debian_folder(root, templ_data, is_python2, cloud_util_deps):
requires = ['cloud-utils | cloud-guest-utils'] if cloud_util_deps else []
# We consolidate all deps as Build-Depends as our package build runs all
# tests so we need all runtime dependencies anyway.
-requires.extend(reqs + test_reqs + [python])
+# NOTE: For some reason python package dependency can't be specified at the
+# end of the dependency list. It results in pybuild errors.
+requires.extend([python] + reqs + test_reqs)
templater.render_to_file(util.abs_join(find_root(),
'packages', 'debian', 'control.in'),
util.abs_join(deb_dir, 'control'),
diff --git a/tools/read-dependencies b/tools/read-dependencies
index 8a58534..ed41e84 100755
--- a/tools/read-dependencies
+++ b/tools/read-dependencies
@@ -40,8 +40,11 @@ DISTRO_INSTALL_PKG_CMD = {
}
-# List of base system packages required to start using make
-EXTRA_SYSTEM_BASE_PKGS = ['make', 'sudo', 'tar']
+# List of base system packages required to enable ci automation
+CI_SYSTEM_BASE_PKGS = {
+'common': ['make', 'sudo', 'tar'],
+'ubuntu': ['devscripts'],
+'debian': ['devscripts']}
# JSON definition of distro-specific package dependencies
@@ -70,10 +73,16 @@ def get_parser():
dest='install',
help='When specified, install the required system packages.')
parser.add_argument(
-'-v', '--python-version', type=str, dest='python_version', default="2",
+'-t', '--test-distro', action='store_true', default=False,
+dest='test_distro',
+help='Additionally install continuous integration system packages '
+ 'required for build and test automation.')
+parser.add_argument(
+'-v', '--python-version', type=str, dest='python_version', default=None,
choices=["2", "3"],
-help='The version of python we want to generate system package '
- 'dependencies for.')
+help='Override the version of python we want to generate system '
+ 'package dependencies for. Defaults to the version of python '
+ 'this script is called with')
return parser
@@ -114,13 +123,17 @@ def parse_pip_requirements(requirements_path):
return dep_names
-def translate_pip_to_system_pkg(pip_requires, renames, python_ver="2"):
+def translate_pip_to_system_pkg(pip_requires, renames, python_ver):
"""Translate pip package names to d
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:ci-deps-fixes into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:ci-deps-fixes into cloud-init:master has been updated. Description changed to: ci dependency installs: Add --test-distro param to read-dependencies to install deps read-dependencies now takes --test-distro param to indicate we want to install all system package depenencies to allow for testing and building for our continous integration environment. It allows us to install all needed deps on a fresh system with python3 ./tools/read-depenencies --distro ubuntu --test-distro [--dry-run]. Additionally read-dependencies now looks at what version of python is running the script (py2 vs p3) and opts to install python 2 or 3 system deps respectively. This behavior can still be overridden with python3 ./tools/read-dependencies ... --python-version 2. There are also some distro-specific packaging dependencies, like devscripts on debian or ubuntu. Those pkg dependencies have now been broken out from common pkg deps to avoid trying to install them on centos/redhat/suse. For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/325693 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:ci-deps-fixes into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master
Looks pretty good, I'd like to see other unit tests specifically looking at
return vals from util.system_info() but that can come in a later branch. Minor
nits but code looks good.
I have no bsd system available otherwise I'd check it out with a couple of runs.
Diff comments:
> diff --git a/cloudinit/util.py b/cloudinit/util.py
> index 415ca37..817d347 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -598,37 +598,29 @@ def get_cfg_option_int(yobj, key, default=0):
> def system_info():
> info = {
> 'platform': platform.platform(),
> +'system': platform.system(),
> 'release': platform.release(),
> 'python': platform.python_version(),
> 'uname': platform.uname(),
> -'dist': platform.linux_distribution(), # pylint: disable=W1505
> +'dist': platform.dist(), # pylint: disable=W1505
> }
> -plat = info['platform'].lower()
> -# Try to get more info about what it actually is, in a format
> -# that we can easily use across linux and variants...
> -if plat.startswith('darwin'):
> -info['variant'] = 'darwin'
> -elif plat.endswith("bsd"):
> -info['variant'] = 'bsd'
> -elif plat.startswith('win'):
> -info['variant'] = 'windows'
> -elif 'linux' in plat:
> -# Try to get a single string out of these...
> -linux_dist, _version, _id = info['dist']
> -linux_dist = linux_dist.lower()
> -if linux_dist in ('ubuntu', 'linuxmint', 'mint'):
> -info['variant'] = 'ubuntu'
> +system = info['system'].lower()
> +var = 'unknown'
> +if system == "linux":
> +linux_dist = info['dist'][0].lower()
> +if linux_dist in ('centos', 'fedora', 'debian'):
do we want a suse case here too?
> +var = linux_dist
> +elif linux_dist in ('ubuntu', 'linuxmint', 'mint'):
> +var = 'ubuntu'
> +elif linux_dist == 'redhat':
> +var = 'rhel'
> else:
> -for prefix, variant in [('redhat', 'rhel'),
> -('centos', 'centos'),
> -('fedora', 'fedora'),
> -('debian', 'debian')]:
> -if linux_dist.startswith(prefix):
> -info['variant'] = variant
> -if 'variant' not in info:
> -info['variant'] = 'linux'
> -if 'variant' not in info:
> -info['variant'] = 'unknown'
> +var = 'linux'
> +elif system in ('windows', 'darwin', "freebsd"):
> +var = system
> +
> +info['variant'] = var
> +
> return info
>
>
> diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
> index 5af2a88..f4b9069 100644
> --- a/config/cloud.cfg.tmpl
> +++ b/config/cloud.cfg.tmpl
> @@ -130,10 +130,8 @@ cloud_final_modules:
> # (not accessible to handlers/transforms)
> system_info:
> # This will affect which distro class gets used
> -{% if variant in ["centos", "debian", "fedora", "rhel", "ubuntu"] %}
> +{% if variant in ["centos", "debian", "fedora", "rhel", "ubuntu", "freebsd"]
> %}
suse in here too?
> distro: {{ variant }}
> -{% elif variant in ["bsd"] %}
> - distro: freebsd
> {% else %}
> # Unknown/fallback distro.
> distro: ubuntu
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325760
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:feature/freebsd-variant into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master
+1 on that suggestion -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325760 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master
The proposal to merge ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master has been updated. Status: Needs review => Approved For more details, see: https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325760 -- Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/freebsd-variant into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:fix/freebsd-test-failure into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceAzure.py
> b/cloudinit/sources/DataSourceAzure.py
> index ebb53d0..e5514ad 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -803,18 +803,22 @@ def encrypt_pass(password, salt_id="$6$"):
> return crypt.crypt(password, salt_id + util.rand_str(strlen=16))
>
>
> +def _check_freebsd_cdrom(cdrom_dev):
> +"""Return boolean indicating path to cdrom device has content."""
Where's the return here? it'll return None in all cases right?
> +try:
> +with open(cdrom_dev) as fp:
> +fp.read(1024)
> +devlist.append(cdrom_dev)
> +except IOError:
> +LOG.debug("cdrom (%s) is not configured", cdrom_dev)
> +
> +
> def list_possible_azure_ds_devs():
> devlist = []
> if util.is_FreeBSD():
> -# add '/dev/cd0' to devlist if it is configured
> -# here wants to test whether '/dev/cd0' is available
> cdrom_dev = "/dev/cd0"
> -try:
> -with open(cdrom_dev) as fp:
> -fp.read(1024)
> -devlist.append(cdrom_dev)
> -except IOError:
> -LOG.debug("cdrom (%s) is not configured", cdrom_dev)
> +if _check_freebsd_cdrom(cdrom_dev):
> +return [cdrom_dev]
> else:
> for fstype in ("iso9660", "udf"):
> devlist.extend(util.find_devs_with("TYPE=%s" % fstype))
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/325771
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:fix/freebsd-test-failure into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:gce-mock-test-leak into cloud-init:master
Chad Smith has proposed merging ~chad.smith/cloud-init:gce-mock-test-leak into
cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327325
test_gce: Fix invalid mock of platform_reports_gce to return False
The mock of platform_reports_gce is created with a True return value in
tests/unittests/test_datasource/test_gce.py:TestDataSourceGCE.setUp(). But, the
final test_get_data_returns_false_if_not_on_gce incorrectly attempts to
override the mocked return_value of True to False by setting
self.m_platform_gce.return_value = False. But, since the mock is already
initialized, the updated False is not honored. Instead we should use the patch
decorator on the specific unit test to override the return_value of
DataSourceGCE.platform_reports_gce to False.
A False from platform_reports_gce allows DataSourceGCE.get_data to immediately
return False instead of trying to contact metadata.google.internal as the
related bug references.
LP:#1703935
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:gce-mock-test-leak into cloud-init:master.
diff --git a/tests/unittests/test_datasource/test_gce.py b/tests/unittests/test_datasource/test_gce.py
index 6fd1341..3e8398b 100644
--- a/tests/unittests/test_datasource/test_gce.py
+++ b/tests/unittests/test_datasource/test_gce.py
@@ -163,8 +163,9 @@ class TestDataSourceGCE(test_helpers.HttprettyTestCase):
self.assertEqual(True, r)
self.assertEqual('bar', self.ds.availability_zone)
-def test_get_data_returns_false_if_not_on_gce(self):
-self.m_platform_reports_gce.return_value = False
+@mock.patch('cloudinit.sources.DataSourceGCE.platform_reports_gce')
+def test_get_data_returns_false_if_not_on_gce(self, m_platform_gce):
+m_platform_gce.return_value = False
self.assertEqual(False, self.ds.get_data())
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:cleanup/ec2-initial-tests into cloud-init:master
Diff comments:
> diff --git a/tests/unittests/test_datasource/test_ec2.py
> b/tests/unittests/test_datasource/test_ec2.py
> new file mode 100644
> index 000..d7362c1
> --- /dev/null
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -0,0 +1,202 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import httpretty
> +import mock
> +
> +from .. import helpers as test_helpers
> +from cloudinit import helpers
> +from cloudinit.sources import DataSourceEc2 as ec2
> +
> +
> +# collected from api version 2009-04-04/ with
> +# python3 -c 'import json
> +# from cloudinit.ec2_utils import get_instance_metadata as gm
> +# print(json.dumps(gm("2009-04-04"), indent=1, sort_keys=True))'
> +DEFAULT_METADATA = {
> +"ami-id": "ami-80861296",
> +"ami-launch-index": "0",
> +"ami-manifest-path": "(unknown)",
> +"block-device-mapping": {"ami": "/dev/sda1", "root": "/dev/sda1"},
> +"hostname": "ip-10-0-0-149",
> +"instance-action": "none",
> +"instance-id": "i-0052913950685138c",
> +"instance-type": "t2.micro",
> +"local-hostname": "ip-10-0-0-149",
> +"local-ipv4": "10.0.0.149",
> +"placement": {"availability-zone": "us-east-1b"},
> +"profile": "default-hvm",
> +"public-hostname": "",
> +"public-ipv4": "107.23.188.247",
> +"public-keys": {"brickies": ["ssh-rsa B3Nzw== brickies"]},
> +"reservation-id": "r-00a2c173fb5782a08",
> +"security-groups": "wide-open"
> +}
> +
> +
> +def register_ssh_keys(rfunc, base_url, keys_data):
It feels like this register_mock_metadata logic is a common tool that other
unit tests outside of Ec2 could benefit from. Any objection to pulling this
content over into tests/unittests/helper.py or maybe creating a
tests/unittests/testing/metadata.py module? Maybe this falls into the category
of "too soon" but it feels like we should be consolidating common unit test
helpers/patterns in a higher level directory or module which has greater
visibility as new unit tests get added.
> +"""handle ssh key inconsistencies.
> +
> +public-keys in the ec2 metadata is inconsistently formated compared
*formatted
> +to other entries.
> +Given keys_data of {name1: pubkey1, name2: pubkey2}
> +
> +This registers the following urls:
> + base_url 0={name1}\n1={name2} # (for each name)
> + base_url/0={name1}\n1={name2} # (for each name)
> + base_url/0 openssh-key
> + base_url/0/ openssh-key
> + base_url/0/openssh-key {pubkey1}
> + base_url/0/openssh-key/ {pubkey1}
> + ...
> +"""
> +
> +base_url = base_url.rstrip("/")
> +odd_index = '\n'.join(
> +["{0}={1}".format(n, name)
> + for n, name in enumerate(sorted(keys_data))])
> +
> +rfunc(base_url, odd_index)
> +rfunc(base_url + "/", odd_index)
> +
> +for n, name in enumerate(sorted(keys_data)):
> +val = keys_data[name]
> +if isinstance(val, list):
> +val = '\n'.join(val)
> +burl = base_url + "/%s" % n
> +rfunc(burl, "openssh-key")
> +rfunc(burl + "/", "openssh-key")
> +rfunc(burl + "/%s/openssh-key" % name, val)
> +rfunc(burl + "/%s/openssh-key/" % name, val)
> +
> +
> +def register_mock_metaserver(base_url, data):
> +"""Register with httpretty a ec2 metadata like service serving 'data'.
> +
> +If given a dictionary, it will populate urls under base_url for
> +that dictionary. For example, input of
> + {"instance-id": "i-abc", "mac": "00:16:3e:00:00:00"}
> +populates
> + base_url with 'instance-id\nmac'
> + base_url/ with 'instance-id\nmac'
> + base_url/instance-id with i-abc
> + base_url/mac with 00:16:3e:00:00:00
> +In the index, references to lists or dictionaries have a trailing /.
> +"""
> +def register_helper(register, base_url, body):
> +base_url = base_url.rstrip("/")
> +if isinstance(body, str):
> +register(base_url, body)
> +elif isinstance(body, list):
> +register(base_url, '\n'.join(body) + '\n')
> +register(base_url + '/', '\n'.join(body) + '\n')
> +elif isinstance(body, dict):
> +vals = []
> +for k, v in body.items():
> +if k == 'public-keys':
> +register_ssh_keys(
> +register, base_url + '/public-keys/', v)
> +continue
> +suffix = k.rstrip("/")
> +if not isinstance(v, (str, list)):
> +suffix += "/"
> +vals.append(suffix)
> +url = base_url + '/' + suffix
> +register_helper(register, url, v)
> +register(base_url, '\n'.join(vals) + '\n')
> +register(base_url + '/', '\n'.join(vals) + '\n')
> +elif body is None:
> +register(base_url, 'not f
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:cleanup/ec2-initial-tests into cloud-init:master
Diff comments:
> diff --git a/tests/unittests/test_datasource/test_ec2.py
> b/tests/unittests/test_datasource/test_ec2.py
> new file mode 100644
> index 000..d7362c1
> --- /dev/null
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -0,0 +1,202 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import httpretty
> +import mock
> +
> +from .. import helpers as test_helpers
> +from cloudinit import helpers
> +from cloudinit.sources import DataSourceEc2 as ec2
> +
> +
> +# collected from api version 2009-04-04/ with
> +# python3 -c 'import json
> +# from cloudinit.ec2_utils import get_instance_metadata as gm
> +# print(json.dumps(gm("2009-04-04"), indent=1, sort_keys=True))'
> +DEFAULT_METADATA = {
> +"ami-id": "ami-80861296",
> +"ami-launch-index": "0",
> +"ami-manifest-path": "(unknown)",
> +"block-device-mapping": {"ami": "/dev/sda1", "root": "/dev/sda1"},
> +"hostname": "ip-10-0-0-149",
> +"instance-action": "none",
> +"instance-id": "i-0052913950685138c",
> +"instance-type": "t2.micro",
> +"local-hostname": "ip-10-0-0-149",
> +"local-ipv4": "10.0.0.149",
> +"placement": {"availability-zone": "us-east-1b"},
> +"profile": "default-hvm",
> +"public-hostname": "",
> +"public-ipv4": "107.23.188.247",
> +"public-keys": {"brickies": ["ssh-rsa B3Nzw== brickies"]},
> +"reservation-id": "r-00a2c173fb5782a08",
> +"security-groups": "wide-open"
> +}
> +
> +
> +def register_ssh_keys(rfunc, base_url, keys_data):
Should this just be a _private function to better designate that it's not
intended for greater unittest consumption, like register_mock_metaserver is?
> +"""handle ssh key inconsistencies.
> +
> +public-keys in the ec2 metadata is inconsistently formated compared
> +to other entries.
> +Given keys_data of {name1: pubkey1, name2: pubkey2}
> +
> +This registers the following urls:
> + base_url 0={name1}\n1={name2} # (for each name)
> + base_url/0={name1}\n1={name2} # (for each name)
> + base_url/0 openssh-key
> + base_url/0/ openssh-key
> + base_url/0/openssh-key {pubkey1}
> + base_url/0/openssh-key/ {pubkey1}
> + ...
> +"""
> +
> +base_url = base_url.rstrip("/")
Since base_url is passed in from register_mock_metaserver we've already
rstripped the trailing slash.
> +odd_index = '\n'.join(
> +["{0}={1}".format(n, name)
> + for n, name in enumerate(sorted(keys_data))])
> +
> +rfunc(base_url, odd_index)
> +rfunc(base_url + "/", odd_index)
> +
> +for n, name in enumerate(sorted(keys_data)):
> +val = keys_data[name]
> +if isinstance(val, list):
> +val = '\n'.join(val)
> +burl = base_url + "/%s" % n
> +rfunc(burl, "openssh-key")
> +rfunc(burl + "/", "openssh-key")
> +rfunc(burl + "/%s/openssh-key" % name, val)
> +rfunc(burl + "/%s/openssh-key/" % name, val)
> +
> +
> +def register_mock_metaserver(base_url, data):
> +"""Register with httpretty a ec2 metadata like service serving 'data'.
> +
> +If given a dictionary, it will populate urls under base_url for
> +that dictionary. For example, input of
> + {"instance-id": "i-abc", "mac": "00:16:3e:00:00:00"}
> +populates
> + base_url with 'instance-id\nmac'
> + base_url/ with 'instance-id\nmac'
> + base_url/instance-id with i-abc
> + base_url/mac with 00:16:3e:00:00:00
> +In the index, references to lists or dictionaries have a trailing /.
> +"""
> +def register_helper(register, base_url, body):
> +base_url = base_url.rstrip("/")
> +if isinstance(body, str):
> +register(base_url, body)
Looks like we've got an inconsistency here for strs. Shouldn't we also register
base_url + '/' too like lists below
> +elif isinstance(body, list):
> +register(base_url, '\n'.join(body) + '\n')
> +register(base_url + '/', '\n'.join(body) + '\n')
> +elif isinstance(body, dict):
> +vals = []
> +for k, v in body.items():
> +if k == 'public-keys':
> +register_ssh_keys(
> +register, base_url + '/public-keys/', v)
> +continue
> +suffix = k.rstrip("/")
> +if not isinstance(v, (str, list)):
> +suffix += "/"
> +vals.append(suffix)
> +url = base_url + '/' + suffix
> +register_helper(register, url, v)
> +register(base_url, '\n'.join(vals) + '\n')
> +register(base_url + '/', '\n'.join(vals) + '\n')
> +elif body is None:
> +register(base_url, 'not found', status_code=404)
> +
> +def myreg(*argc, **kwargs):
> +# print("register_url(%s, %s)" % (argc, kwargs))
> +
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master
Chad Smith has proposed merging
~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327827
cloudinit.net: add initialize_network_device function and unittest improvements.
This is not yet called, but will be called in a subsequent Ec2-related branch
to manually initialize a network interface with the responses using dhcp
discovery without any dhcp-script side-effects. The functionality has been
tested on Ec2 ubuntu and CentOS vms to ensure that network interface
initialization works in both OS-types.
Since there was poor unit test coverage for the cloudinit.net.__init__ module,
this branch adds a bunch of coverage to the functions in cloudinit.net.__init.
We can also now have unit tests local to the cloudinit modules. The benefits of
having unittests under cloudinit module:
- Proximity of unittest to cloudinit module makes it easier for ongoing devs
to know where to augment unit tests. The tests.unittest directory is
organizated such that it
- Allows for 1 to 1 name mapping module -> tests/test_module.py
- Improved test and module isolation, if we find unit tests have to import
from a number of modules besides the module under test, it will better prompt
resturcturing of the module.
This also branch touches:
- tox.ini to run unit tests found in cloudinit as well as include mock
test-requirement for pylint since we now have unit tests living within
cloudinit package
- setup.py to exclude any test modules under cloudinit when packaging
To test:
make deb
dpkg -c cloud-init_all.deb | grep test # make sure our package didn't
include tests
tox
# on an lxc: ifconfig eth0 0.0.0.0; python -c 'from cloudinit.net import
initialize_network_device; initialize_network_device("eth0", "ip-addr",
"netmask", "broadcast")'
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master.
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index d1740e5..5a4a232 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -10,6 +10,7 @@ import logging
import os
import re
+from cloudinit.net.network_state import mask_to_net_prefix
from cloudinit import util
LOG = logging.getLogger(__name__)
@@ -77,7 +78,7 @@ def read_sys_net_int(iface, field):
return None
try:
return int(val)
-except TypeError:
+except ValueError:
return None
@@ -149,7 +150,14 @@ def device_devid(devname):
def get_devicelist():
-return os.listdir(SYS_CLASS_NET)
+try:
+devs = os.listdir(SYS_CLASS_NET)
+except OSError as e:
+if e.errno == errno.ENOENT:
+devs = []
+else:
+raise
+return devs
class ParserError(Exception):
@@ -497,14 +505,8 @@ def get_interfaces_by_mac():
"""Build a dictionary of tuples {mac: name}.
Bridges and any devices that have a 'stolen' mac are excluded."""
-try:
-devs = get_devicelist()
-except OSError as e:
-if e.errno == errno.ENOENT:
-devs = []
-else:
-raise
ret = {}
+devs = get_devicelist()
empty_mac = '00:00:00:00:00:00'
for name in devs:
if not interface_has_own_mac(name):
@@ -531,14 +533,8 @@ def get_interfaces():
"""Return list of interface tuples (name, mac, driver, device_id)
Bridges and any devices that have a 'stolen' mac are excluded."""
-try:
-devs = get_devicelist()
-except OSError as e:
-if e.errno == errno.ENOENT:
-devs = []
-else:
-raise
ret = []
+devs = get_devicelist()
empty_mac = '00:00:00:00:00:00'
for name in devs:
if not interface_has_own_mac(name):
@@ -557,6 +553,27 @@ def get_interfaces():
return ret
+def initialize_network_device(interface, ip_address, netmask, broadcast,
+ router=None):
+if not all([interface, ip_address, netmask, broadcast]):
+raise ValueError(
+"Cannot init network dev {0} with ip{1}/{2} and bcast {3}".format(
+interface, ip_address, netmask, broadcast))
+prefix = mask_to_net_prefix(netmask)
+
+util.subp([
+'ip', '-family', 'inet', 'addr', 'add', '%s/%s' % (ip_address, prefix),
+'broadcast', broadcast, 'dev', interface], capture=True)
+util.subp(
+['ip', '-family', 'inet', 'link', 'set', 'dev', interface, 'up
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master has been updated. Commit Message changed to: cloudinit.net: add initialize_network_device function and unittests This is not yet called, but will be called in a subsequent Ec2-related branch to manually initialize a network interface with the responses using dhcp discovery without any dhcp-script side-effects. The functionality has been tested on Ec2 ubuntu and CentOS vms to ensure that network interface initialization works in both OS-types. Since there was poor unit test coverage for the cloudinit.net.__init__ module, this branch adds a bunch of coverage to the functions in cloudinit.net.__init. We can also now have unit tests local to the cloudinit modules. The benefits of having unittests under cloudinit module: - Proximity of unittest to cloudinit module makes it easier for ongoing devs to know where to augment unit tests. The tests.unittest directory is organizated such that it - Allows for 1 to 1 name mapping module -> tests/test_module.py - Improved test and module isolation, if we find unit tests have to import from a number of modules besides the module under test, it will better prompt resturcturing of the module. This also branch touches: - tox.ini to run unit tests found in cloudinit as well as include mock test-requirement for pylint since we now have unit tests living within cloudinit package - setup.py to exclude any test modules under cloudinit when packaging For more details, see: https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327827 -- Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:unittests-in-cloudinit-package
into cloud-init:master has been updated.
Description changed to:
cloudinit.net: add initialize_network_device function and unittest.
This is not yet called, but will be called in a subsequent Ec2-related branch
to manually initialize a network interface with the responses using dhcp
discovery without any dhcp-script side-effects. The functionality has been
tested on Ec2 ubuntu and CentOS vms to ensure that network interface
initialization works in both OS-types.
Since there was poor unit test coverage for the cloudinit.net.__init__ module,
this branch adds a bunch of coverage to the functions in cloudinit.net.__init.
We can also now have unit tests local to the cloudinit modules. The benefits of
having unittests under cloudinit module:
- Proximity of unittest to cloudinit module makes it easier for ongoing devs
to know where to augment unit tests. The tests.unittest directory is
organizated such that it
- Allows for 1 to 1 name mapping module -> tests/test_module.py
- Improved test and module isolation, if we find unit tests have to import
from a number of modules besides the module under test, it will better prompt
resturcturing of the module.
This also branch touches:
- tox.ini to run unit tests found in cloudinit as well as include mock
test-requirement for pylint since we now have unit tests living within
cloudinit package
- setup.py to exclude any test modules under cloudinit when packaging
To test:
make deb
dpkg -c cloud-init_all.deb | grep test # make sure our package didn't
include tests
tox
# on an lxc: ifconfig eth0 0.0.0.0; python -c 'from cloudinit.net import
initialize_network_device; initialize_network_device("eth0", "ip-addr",
"netmask", "broadcast")'
For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327827
--
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master
Diff comments: > diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py > index d1740e5..5a4a232 100644 > --- a/cloudinit/net/__init__.py > +++ b/cloudinit/net/__init__.py > @@ -77,7 +78,7 @@ def read_sys_net_int(iface, field): > return None > try: > return int(val) > -except TypeError: > +except ValueError: int() raises a ValueError not a TypeError. > return None > > > @@ -149,7 +150,14 @@ def device_devid(devname): > > > def get_devicelist(): > -return os.listdir(SYS_CLASS_NET) > +try: Pulled this common exception handling logic into get_devicelist so it's not replicated at call sites. > +devs = os.listdir(SYS_CLASS_NET) > +except OSError as e: > +if e.errno == errno.ENOENT: > +devs = [] > +else: > +raise > +return devs > > > class ParserError(Exception): -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327827 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
