LGTM.

Thanks,
Jose

On Wed, Feb 05, 2014 at 07:15:43PM +0100, Santi Raffa wrote:
> On Wed, Feb 5, 2014 at 6:29 PM, Jose A. Lopes <[email protected]> wrote:
> > Also, you have several patches like
> >   modify InstanceCreate
> >   modify InstanceReinstall
> >   modify InstanceClusterSetParams
> >   modify InstanceSetParams
> > but the command line options seem to be split between this patch and
> > the rest.  Is there a reason behind this?
> 
> I'm defining them now but using them later mostly for ease of authoring.
> 
> Here's the current and future interdiffs.
> 
> diff --git a/lib/client/gnt_os.py b/lib/client/gnt_os.py
> index c2e5a1b..ae7b9eb 100644
> --- a/lib/client/gnt_os.py
> +++ b/lib/client/gnt_os.py
> @@ -294,7 +294,7 @@ commands = {
>      "operating systems"),
>    "modify": (
>      ModifyOS, ARGS_ONE_OS,
> -    [HVLIST_OPT, OSPARAMS_OPT, OSPARAMS_NOLOG_OPT,
> +    [HVLIST_OPT, OSPARAMS_OPT, OSPARAMS_PRIVATE_OPT,
>       DRY_RUN_OPT, PRIORITY_OPT, HID_OS_OPT, BLK_OS_OPT] + SUBMIT_OPTS,
>      "", "Modify the OS parameters"),
>    }
> 
> diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
> index b4deaa7..4a7f71b 100644
> --- a/lib/client/gnt_instance.py
> +++ b/lib/client/gnt_instance.py
> @@ -1485,8 +1485,7 @@ add_opts = [
>  commands = {
>    "add": (
>      AddInstance, [ArgHost(min=1, max=1)],
> -    COMMON_CREATE_OPTS + add_opts + [OSPARAMS_NOLOG_OPT,
> -                                     OSPARAMS_NOLOG_NOSAVE_OPT],
> +    COMMON_CREATE_OPTS + add_opts + [OSPARAMS_PRIVATE_OPT,
> OSPARAMS_SECRET_OPT],
>      "[...] -t disk-type -n node[:secondary-node] -o os-type <name>",
>      "Creates and adds a new instance to the cluster"),
>    "batch-create": (
> @@ -1549,7 +1548,7 @@ commands = {
>       m_pri_node_opt, m_sec_node_opt, m_clust_opt, m_inst_opt, 
> m_node_tags_opt,
>       m_pri_node_tags_opt, m_sec_node_tags_opt, m_inst_tags_opt, 
> SELECT_OS_OPT]
>      + SUBMIT_OPTS + [DRY_RUN_OPT, PRIORITY_OPT, OSPARAMS_OPT,
> -                     OSPARAMS_NOLOG_OPT, OSPARAMS_NOLOG_NOSAVE_OPT],
> +                     OSPARAMS_PRIVATE_OPT, OSPARAMS_SECRET_OPT],
>      "[-f] <instance>", "Reinstall a stopped instance"),
>    "remove": (
>      RemoveInstance, ARGS_ONE_INSTANCE,
> @@ -1573,7 +1572,7 @@ commands = {
>      SetInstanceParams, ARGS_ONE_INSTANCE,
>      [BACKEND_OPT, DISK_OPT, FORCE_OPT, HVOPTS_OPT, NET_OPT] + SUBMIT_OPTS +
>      [DISK_TEMPLATE_OPT, SINGLE_NODE_OPT, OS_OPT, FORCE_VARIANT_OPT,
> -     OSPARAMS_OPT, OSPARAMS_NOLOG_OPT, DRY_RUN_OPT, PRIORITY_OPT, NWSYNC_OPT,
> +     OSPARAMS_OPT, OSPARAMS_PRIVATE_OPT, DRY_RUN_OPT, PRIORITY_OPT, 
> NWSYNC_OPT,
>       OFFLINE_INST_OPT, ONLINE_INST_OPT, IGNORE_IPOLICY_OPT, RUNTIME_MEM_OPT,
>       NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT, HOTPLUG_OPT,
>       HOTPLUG_IF_POSSIBLE_OPT],
> 
> diff --git a/lib/cli.py b/lib/cli.py
> index e257b4e..04291c9 100644
> --- a/lib/cli.py
> +++ b/lib/cli.py
> @@ -164,8 +164,8 @@ __all__ = [
>    "ON_SECONDARY_OPT",
>    "OFFLINE_OPT",
>    "OSPARAMS_OPT",
> -  "OSPARAMS_NOLOG_OPT",
> -  "OSPARAMS_NOLOG_NOSAVE_OPT",
> +  "OSPARAMS_PRIVATE_OPT",
> +  "OSPARAMS_SECRET_OPT",
>    "OS_OPT",
>    "OS_SIZE_OPT",
>    "OOB_TIMEOUT_OPT",
> @@ -949,19 +949,20 @@ OSPARAMS_OPT = cli_option("-O",
> "--os-parameters", dest="osparams",
>                            type="keyval", default={},
>                            help="OS parameters")
> 
> -OSPARAMS_NOLOG_OPT = cli_option("--os-parameters-private",
> -                                dest="osparams_private",
> -                                type="keyprivateval",
> -                                default=serializer.PrivateDict(),
> -                                help="Private OS parameters (won't be 
> logged)")
> -
> -OSPARAMS_NOLOG_NOSAVE_OPT = cli_option("--os-parameters-secret",
> -                                       dest="osparams_secret",
> -                                       type="keyprivateval",
> -                                       default=serializer.PrivateDict(),
> -                                       help="Secret OS parameters (won't be"
> -                                            " logged or saved; must supply"
> -                                            " every time.)")
> +OSPARAMS_PRIVATE_OPT = cli_option("--os-parameters-private",
> +                                  dest="osparams_private",
> +                                  type="keyprivateval",
> +                                  default=serializer.PrivateDict(),
> +                                  help="Private OS parameters"
> +                                       " (won't be logged)")
> +
> +OSPARAMS_SECRET_OPT = cli_option("--os-parameters-secret",
> +                                 dest="osparams_secret",
> +                                 type="keyprivateval",
> +                                 default=serializer.PrivateDict(),
> +                                 help="Secret OS parameters (won't be
> logged or"
> +                                      " saved; you must supply these for 
> every"
> +                                      " operation.)")
> 
>  FORCE_VARIANT_OPT = cli_option("--force-variant", dest="force_variant",
>                                 action="store_true", default=False,
> 
> 
> 
> -- 
> Raffa Santi
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
> 
> 
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores

-- 
Jose Antonio Lopes
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370

Reply via email to