Code review comment for lp:~ltrager/maas/reset_node

Hehe se actually discussed it yesterday and came to the conclusion that we
need to change reset to restore and have individual
Methods for storage and networking!

On Friday, April 29, 2016, Gavin Panella <email address hidden>
wrote:

> Review: Approve
>
> [I wrote some comments on this yesterday and forgot to save them.]
>
> Looks good to me. I've made some comments about `idempotent` which need
> addressing, but there's no need for me to block you on that.
>
> You've just got to convince roaksoax now :)
>
> Diff comments:
>
> > === modified file 'src/maasserver/api/devices.py'
> > --- src/maasserver/api/devices.py 2016-04-14 15:25:36 +0000
> > +++ src/maasserver/api/devices.py 2016-04-28 09:04:07 +0000
> > @@ -117,6 +118,22 @@
> > device.delete()
> > return rc.DELETED
> >
> > + @operation(idempotent=True)
>
> I chose the argument name "idempotent" and unfortunately it wasn't a good
> choice. This reset operation is idempotent but it's also destructive, so
> this should be specified as idempotent=False. Ultimately this is used to
> specify an operation as available via GET (idempotent=True) or POST
> (idempotent=False), and this operation should definitely only be available
> via POST.
>
> > + def reset(self, request, system_id):
> > + """Reset a machine's network and storage options.
> > +
> > + Resets a machine's network and storage options as if the node
> was just
> > + commissioned.
> > +
> > + Returns 404 if the machine is not found
> > + Returns 403 if the user does not have permission to reset the
> machine.
>
> For this whole docstring, s/machine/device/ and remove mentions of storage
> options.
>
> > + """
> > + device = self.model.objects.get_node_or_404(
> > + system_id=system_id, user=request.user,
> > + perm=NODE_PERMISSION.ADMIN)
> > + device.set_initial_networking_configuration()
> > + return reload_object(device)
> > +
> > @operation(idempotent=False)
> > def set_owner_data(self, request, system_id):
> > """Set key/value data for the current owner.
> >
> > === modified file 'src/maasserver/api/machines.py'
> > --- src/maasserver/api/machines.py 2016-04-15 22:14:33 +0000
> > +++ src/maasserver/api/machines.py 2016-04-28 09:04:07 +0000
> > @@ -697,6 +700,22 @@
> > request.user.username)
> > return node
> >
> > + @operation(idempotent=True)
>
> Same here, re. idempotent.
>
> > + def reset(self, request, system_id):
> > + """Reset a machine's network and storage options.
> > +
> > + Resets a machine's network and storage options as if the node
> was just
> > + commissioned.
> > +
> > + Returns 404 if the machine is not found.
> > + Returns 403 if the user does not have permission to reset the
> machine.
> > + """
> > + machine = self.model.objects.get_node_or_404(
> > + system_id=system_id, user=request.user,
> > + perm=NODE_PERMISSION.ADMIN)
> > + machine.reset()
> > + return reload_object(machine)
> > +
> > @operation(idempotent=False)
> > def set_owner_data(self, request, system_id):
> > """Set key/value data for the current owner.
>
>
> --
> https://code.launchpad.net/~ltrager/maas/reset_node/+merge/293222
> You are reviewing the proposed merge of lp:~ltrager/maas/reset_node into
> lp:maas.
>

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer