[Piuparts-devel] Bug#893731: Bug#893731: [piuparts] Please add docker support

[Agustin Henze]

On 03/23/18 15:03, Holger Levsen wrote:
[snip]
> 
> I've done some review...
> 
>>  *--schroot*='SCHROOT-NAME'::
>> -  Use schroot session named SCHROOT-NAME for the chroot, instead of building a new one with debootstrap.
>> +  Use schroot session named SCHROOT-NAME for the testing environment.
> 
> why drop the second half here?

Not really sure, maybe I thought it was more clear. But now you mention it, I
prefer to keep it hehe. I've re-added it

> 
>>      parser.add_option("--schroot", metavar="SCHROOT-NAME", action="store",
>> -                      help="Use schroot session named SCHROOT-NAME for the chroot, instead of building " +
>> -                           "a new one with debootstrap.")
>> +                      help="Use schroot session named SCHROOT-NAME for the testing environment.")
>

done

> same
> 
>>  *-k*, *--keep-tmpdir*::
>> -  Don't remove the temporary directory for the chroot when the program ends.
>> +  Depending on which option is passed, it keeps the environment used for testing after the program ends::
>> +   * By default it doesn't remove the temporary directory for the chroot
>> +   * if --schroot is used, the schroot session is not terminated
>> +   * or if --docker-image is used, the container created is not destroyed.
>  
> I think I prefer:
> 
> Depending on which option is passed, it keeps the environment used for testing after the program ends::
>  * By default it doesn't remove the temporary directory for the chroot,
>  * or if --schroot is used, the schroot session is not terminated,
>  * or if --docker-image is used, the container created is not destroyed.

Done

> (twice "or if" at the beginning and always commas at the end)
> 
>> -                      help="Don't remove the temporary directory for the " +
>> -                           "chroot when the program ends.")
>> +                      help="It keeps the environment used for testing after "
>> +                      "the program ends.")
> 
> -> "Keep the environment used for testing after the programm ends."

Done

>> --- a/piuparts.py
>> +++ b/piuparts.py
> [...]
>> +import json
> 
> doesn't this need a new dependency?

Noup, it's part of the stdlib
libpython3.6-stdlib:amd64: /usr/lib/python3.6/json/__init__.py
libpython2.7-stdlib:amd64: /usr/lib/python2.7/json/__init__.py

> 
>>      def create_resolv_conf(self):
>> +        if settings.docker_image:
>> +            # Do nothing, docker already takes care of this
>> +            return
> 
> and
> 
>>      def terminate_running_processes(self):
>> +        if settings.docker_image:
>> +            # docker takes care of this
>> +            return
> 
> here I wonder: is it really cleaner to return if docker is used or
> wouldnt it be better to not call it in the first place?

Maybe, I'm not totally sure, but if you feel it's better, I prefer the way you
feel more comfortable :). After all, it's your little creature hehe

> 
> The rest looks fine to me and I'll happily merge once you fixed up those
> little things. Thanks again!

Great! Please take a look at it again. Also I've made little fixes that Andreas
Beckman and Iñaki Malerva point me on the PR ¯\_(ツ)_/¯. Basically add docker.io
as suggested package, edit debian/changelog and fix a typo in doc.

Thanks,

-- 
TiN
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-docker-support-new-param-is-introduced-docker-im.patch
Type: text/x-patch
Size: 9180 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-docker-image-param-doc-in-manpage.patch
Type: text/x-patch
Size: 1175 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Rewrite-keep-tmpdir-doc.patch
Type: text/x-patch
Size: 1931 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Rephrase-schroot-param-doc.patch
Type: text/x-patch
Size: 1969 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Add-docker.io-as-suggested-package.patch
Type: text/x-patch
Size: 763 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Update-debian-changelog.patch
Type: text/x-patch
Size: 992 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180324/3e52cfdc/attachment-0001.sig>