On 12/02, Brian Foley wrote:
> On Thu, Dec 01, 2016 at 12:34:11PM +0200, Yiannis Tsiouris wrote:
> > This patch extends 'gnt-instance modify' by allowing a user to remove a
> > list of public/private OS parameters from an instance. This can be
> > useful before performing a reinstall to a new OS provider. Example
> > usage:
> >
> > $ gnt-instance modify --remove-os-parameters parm1,parm2 <instance_name>
> >
> > or
> >
> > $ gnt-instance modify --remove-os-parameters-private parm3 <instance_name>
> >
> > Signed-off-by: Yiannis Tsiouris <ts...@grnet.gr>
>
> Mostly LGTM, see a couple of inline comments.
I agree with everything.
> Also, a thought: I wonder could we merge the --clear-os-parameters
> functionality
> into --remove-os-parameters by having a special '*' parameter. If it's the
> only
> one present in the list, it means remove everything. It would mean we could
> get
> rid of all the boilerplate associated with parsing (and serialising) 2 extra
> options.
Hmm, that's an interesting point. I think this could be more clear both for the
user and the code. I could do that! Just to be clear: I should treat the string
'*' as a "special" OS parameter; this doesn't mean that I should try to globe
based on '*' i.e. 'param*' doesn't expand to 'param1', 'param2', etc., right?
Thanks,
Yiannis
> > + for osp in remove_osparams:
> > + if osp in public_parms:
> > + raise errors.OpPrereqError("Requested both removal and addition
> > of "
> > + "param %s" % osp)
> > +
> > + if osp in self.instance.osparams:
> > + self.os_inst_removed.append(osp)
> > + del self.instance.osparams[osp]
>
> Maybe add an else: print warning about removing an option that's not set.
>
> > +
> > + for osp in remove_osparams_private:
> > + if osp in private_parms:
> > + raise errors.OpPrereqError("Requested both removal and addition
> > of "
> > + "param %s" % osp)
> > +
> > + if osp in self.instance.osparams_private:
> > + self.os_inst_private_removed.append(osp)
> > + del self.instance.osparams_private[osp]
>
> Warn here too.
>
