cloud-init-dev team mailing list archive

[Wesley Wiedenmeier <wesley.wiedenmeier@xxxxxxxxx>]

> lots of comments...
> Thanks for all the work here, we need to try to do things more
> iteratively. I know we're still bringing this all up, but there
> is just so much here that its almost impossible to review.

Yeah this was definitely too large a merge, we just got stuck with
lots of changes that depend on each other. This replaces a pretty
huge chunk of the current codebase as most of that was just
temporary to get some tests running while the main part of this
got finished.

> General:
>  * please lets try to do smaller merge proposals, doing one set
>    of things at a time.

Definitely, this should be the last major merge, since all of the
pieces that had to be rewritten are finished. Next merge should just
be the new platform.

>  * full chart of enabled/disabled at: http://goo.gl/q78sY8
>    this is a private url, not suitable for a commit message
>    possibly we just move this into doc in cloud-init?

I'll go ahead and pull this link out of the commit message. I have
been meaning to do some doc updates, I'll add a link to the
spreadsheet and make it public when I put together the merge for
that.

>  * where is the upstream bug that prevents 2.2 from working
>    correctly?

It was at https://github.com/lxc/pylxd/issues/209, but the fix has
been pulled in now. I'm going to set up a ppa to build a version with
the fix for devel use. However, for now using 2.1.3 works for
jenkins, the only disadvantage there is that if anything goes wrong
during setup it will fail silently. This version of the code will run
with either 2.1.3 or 2.2, so when the fix is released all we have to
do to switch is change the version in the tox env.

>  * i think we can live without 'alias: t'.

The 'alias' key is used to reference the image, on the ubuntu daily
sstream server the aliases I found were p, t, x, y, z. It's now been
switched over to ubuntu/trusty/default with the switch to images from
images.linuxcontainers.org.

>  * apparently we should be using dnf rather than yum in any recent
>    RH distro

Oh, I didn't know that dnf was officially supported outside Fedora.
It shouldn't take much to switch that back over, just gotta update
the setup image functions.

>  * "switch to using images.linuxcontainers.org from using ubuntu
>     daily images" why ?? we really need to be testing Ubuntu
>     images, not linuxcontainers.org I like using them for other
>     distros where we have nothing else, but ubuntu should test
>     primarily on ubuntu images.

I had mainly done it for consistancy across the different distros and
because linuxcontainers images download way faster than ubuntu daily
images (at least for me) but its just a config change to switch back
to the ubuntu daily repo for ubuntu images. I can switch that back
tonight.

>  * system_ready_script can be simplified by being a command,
>    passed directly to 'execute'.
>      system_ready_script: ['test', '-e',
>                            '/run/cloud-init/result.json']
>    that saves writing the file and executing it on the other
>    side, and also then assuming 'bash'.

That makes sense, although it does limit how complex the scripts can
get. Its probably best to keep them simple though.

>  * "Pass $HOME through in tox citest envs for lxc client use"
>    message doesn't make sense. where are we using the lxc client? just
>    for developer debug ?

It is used for

>  * "Disable centos66 test"
>    why ?

Its such an old release that pretty much nothing is going to be
backported there, testing on it doesn't really make sense IMO. I
think the one time I tried running centos 6 tests they failed, but I
don't recall why. I can get tests going there in the future if
they're needed.

> tox.ini:
>  * multiple citest_* are fine for now, but i really do not want them
>    going forward.

They can be removed without doing any harm, only the 'citest' one
will actually be used by jenkins. The 'citest_new_pylxd' env is just
for testing while developing tests, until the new release in the 2.2
series. The 'citest_tree_run' env is just shorthand for calling
'tox -e citest -- tree_run -v ...', it isn't essential.

> Easily droppable commits / things that can be done separartely:
> - "Use LOG.warning instead of deprectated LOG.warn"
>    why? This is just not necessary in this merge proposal.
> - "Change behavior of upgrade in setup_image"

The old behavior of update was a bug, it was fixed before the
original merge, but didn't make it in. Tests that pull from repos
are very slow without the new behavior, and are unreliable
if -proposed is enabled.

The LOG.warning/warn switch was just to silence a deprecation
warning. It isn't really essential. I could rebase and pull some of
the little commits together into one general purpose cleanup commit
to clean up the log. Some of this definitely could have been done
in other merges, I'll do it like that in the future.

> tests/cloud_tests/bddeb.py
>  * 7cd4a3b5 source_archive: can we either
>    a.) just assume the deb-src lines are present
>    b.) hueristically grab one... i'd like to not use us.archive.com
>        if the image is otherwise using archive.ubuntu.com as that
>        will avoid any proxy in the path. this is kind of tough
>        actually..

This issue goes away if we switch back to using ubuntu daily images
for ubuntu releases as they already have deb-src entries. This was
just a workaround because of the linuxcontainers.org images, but
since its better to use ubuntu daily this can just be removed.

>  * creating the tarball..
>    I think what we want to do here is:
>      a.) make-tarball on "this side"
>      b.) modify bddeb to be able to just create a debian/ dir
>          either
>          i.) have it create the orig tarball and a debian.tar.gz
>              into a provided (empty) output dir.
>          ii.) have it just create the debian.tar.gz and we create
>              the tarball example of doing that
>              http://paste.ubuntu.com/23870964/
>      c.) move the tarball and debian dir over, extract tarball,
>         extract debian/
>      d.) mk-build-deps --install .... debian/control
>      e.) dpkg-buildpackage
> 
>    doing this should mean we do  not have to 'install additional
>    build deps' but rather just have mk-build-deps do the right
>    thing.

I had been trying to avoid modifying anything outside cloud_tests,
but adding the --debian-tar-only option in bddeb definitely does
simplify this. This may be a useful feature in general for debugging
cloud-init builds that do not work or doing custom deb builds, as
going through the template system can be inconvenient.

This also avoids having to go through a source repo in case the
current version of cloud-init in tree has different deps from the
one in the source repo.

Using dpkg-buildpackage instead of debuild may be quicker too.

>  * when building with dpkg-buildpackage or debuild, good to
>    allow setting DEB_BUILD_OPTIONS=nocheck (which will not run pep8 and
>    nosetests)

Nice, I didn't know that was an option. Lets us avoid the additional
build deps step easily, since that was mostly to avoid failing because
testing stuff wasn't there.

>  * i think we can drop 'git' due to above, 'tar' is essential so you
>    do not have to declare it.
>    lets do the install of devscripts and equivs with
>    --no-install-recommends and also eatmydata everywhere

Yeah, that should save some time getting everything set up.

>  * i think the above path means we do not depend on deb-src lines.

The switch back to ubuntu daily will handle that, but this also
avoids going through source repo at all which can be nice if a local
change adds a new build dep.
-- 
https://code.launchpad.net/~wesley-wiedenmeier/cloud-init/+git/cloud-init/+merge/308218
Your team cloud init development team is requested to review the proposed merge of ~wesley-wiedenmeier/cloud-init:integration-testing into cloud-init:master.