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

[Holger Levsen]

On Thu, Mar 22, 2018 at 09:36:46AM -0300, Agustin Henze wrote:
> I have added doc for the new param and reworded some others :), please find the
> patches attached. I have pushed to the PR[0] as well because IMO it's easier
> for doing review there. 

yep, many thanks, also for the updated/new patches!

> It'd be awesome if you migrate the project to salsa,

I will do, latest til the end of May ;-)

> Also, I tested a little bit more using no option, --schroot and the new one
> --docker-image. I'd be really happy if you do some testing on your side and
> give me some feedback.

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?

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

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.

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

> --- a/piuparts.py
> +++ b/piuparts.py
[...]
> +import json

doesn't this need a new dependency?

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



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


-- 
cheers,
	Holger
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20180323/65555fc6/attachment.sig>