curtin-dev team mailing list archive

[Olivier Gayot <mp+444008@xxxxxxxxxxxxxxxxxx>]

Thanks!

Would you update doc/topics/config.rst?

Also, I really like the 'extra-rsync-args' spelling a lot more than 'extra_rsync_args'. That said, all other configuration directives under 'install' are spelled like the latter (i.e., with underscores to separate words) so it feels a bit out of place at the moment.

Do we have a plan going forward?

I'd like to deprecate all `configuration_directives` in favor of `configuration-directives` using code like:

resume_data = install_cfg.get('resume-data', install_cfg.get('resume_data'))
save_install_logs = install_cfg.get('save-install-logs', install_cfg.get('save_install_logs'))
...

What do you think?

Diff comments:

> diff --git a/curtin/commands/extract.py b/curtin/commands/extract.py
> index bfcb076..7df4242 100644
> --- a/curtin/commands/extract.py
> +++ b/curtin/commands/extract.py
> @@ -199,27 +199,31 @@ def get_handler_for_source(source):
>          return None
>  
>  
> -def extract_source(source, target):
> +def extract_source(install_cfg, source, target):
>      handler = get_handler_for_source(source)
>      if handler is not None:
>          root_dir = handler.setup()
>          try:
> -            copy_to_target(root_dir, target)
> +            copy_to_target(install_cfg, root_dir, target)
>          finally:
>              handler.cleanup()
>      else:
>          extract_root_tgz_url(source['uri'], target=target)
>  
>  
> -def copy_to_target(source, target):
> +def copy_to_target(install_cfg, source, target):

Is it worth it to take the full install_cfg object here?

I would suggest something like:

def copy_to_target(source, target, *, extra_rsync_args=None):
    if extra_rsync_args is None:
        extra_rsync_args = []

or with type hints

def copy_to_target(source, target, *, extra_rsync_args: Optional[List[str]] = None)

>      if source.startswith("cp://"):
>          source = source[5:]
>      source = os.path.abspath(source)
>  
> -    util.subp(args=['sh', '-c',
> -                    ('mkdir -p "$2" && cd "$2" && '
> -                     'rsync -aXHAS --one-file-system "$1/" .'),
> -                    '--', source, target])
> +    os.makedirs(target, exist_ok=True)
> +
> +    util.subp(
> +        args=[

minor: it's not new but I don't think using a named argument for `args` brings any benefit.

util.subp(['rsync', ...], cwd=target)

feels more natural IMO - but feel free to ignore.

> +            'rsync', '-aXHAS', '--one-file-system'
> +            ] + install_cfg.get('extra-rsync-args', []) + [
> +            source + '/', '.'],
> +        cwd=target)
>  
>  
>  def _path_from_file_url(url):


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/444008
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:extra-rsync-args into curtin:master.