Code review comment for ~chad.smith/cloud-init:cyaml-loading

Revision history for this message
Scott Moser (smoser) wrote :

> 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

I don't follow this. In ubuntu at least cloud-init does depend on python3-yaml and python3-yaml on libyaml-0-2.

$ apt-cache show python3-yaml | grep Dep
Depends: python3 (<< 3.6), python3 (>= 3.5~), libc6 (>= 2.14), libyaml-0-2

There is no reason to explicitly depend on libyaml-0-2, and we have a fallback
path in the code.

Why did you think we need python-devel ? This is where tox falls apart honestly, when you get into C python extensions. But we already have some of those in the test path (pylxd).

> 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

if we do skipIf, then the package build environment will run though the CLoader path, and the tox path would not. We could put in some way of failing if the package build environment didn't take the CLoader path. That'd ensure that we were getting tests on both paths.

I think that might be over-engineering.

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

I think over-engineering, especially for the size of the gain. Its entirely possible that there is overhead of loading c that makes this path slower, meaning we'd be doing this work for negative result.

Best case, i think the amount of yaml that cloud-init loads is probably small enough to look like noise here.

« Back to merge proposal