Code review comment for ~mpontillo/maas:add-deploy-kvm-ui--bug-1795013

We should send this over to UX team for a quick view.

On Tue, Oct 9, 2018 at 9:37 PM Kit Randel <email address hidden> wrote:

> Review: Approve
>
> code lgtm, just a few comments inline.
>
> Diff comments:
>
> > diff --git
> a/src/maasserver/static/js/angular/controllers/node_details.js
> b/src/maasserver/static/js/angular/controllers/node_details.js
> > index 55d1c16..f4083b5 100644
> > --- a/src/maasserver/static/js/angular/controllers/node_details.js
> > +++ b/src/maasserver/static/js/angular/controllers/node_details.js
> > @@ -139,9 +142,9 @@
> angular.module('MAAS').controller('NodeDetailsController', [
> > // id matching the node id. Otherwise we end up setting our
> > // selected field to an option not from DomainsManager which
> > // doesn't work.
> > - var i;
> > + let i;
> > for(i=0;i<$scope.header.domain.options.length;i++) {
>
> Can `i` be initialised inside the `for` like the other cases?
>
> > - var option = $scope.header.domain.options[i];
> > + let option = $scope.header.domain.options[i];
> > if(option.id === $scope.node.domain.id) {
> > $scope.header.domain.selected = option;
> > break;
> > @@ -592,13 +592,14 @@
> angular.module('MAAS').controller('NodeDetailsController', [
> > $scope.osSelection.hwe_kernel.indexOf('ga-') >= 0))
> {
> > extra.hwe_kernel = $scope.osSelection.hwe_kernel;
> > }
> > + extra.install_kvm = $scope.deployOptions.installKVM;
> > } else if($scope.action.option.name === "commission") {
> > extra.enable_ssh = $scope.commissionOptions.enableSSH;
> > extra.skip_networking = (
> > $scope.commissionOptions.skipNetworking);
> > extra.skip_storage =
> $scope.commissionOptions.skipStorage;
> > extra.commissioning_scripts = [];
> > - for(i=0;i<$scope.commissioningSelection.length;i++) {
> > + for(let i=0;i<$scope.commissioningSelection.length;i++)
> {
>
> A minor point, but this is a bit more readable with spaces around the
> operators and after the semi-colons (like l93).
>
> > extra.commissioning_scripts.push(
> > $scope.commissioningSelection[i].id);
> > }
>
>
> --
> https://code.launchpad.net/~mpontillo/maas/+git/maas/+merge/356376
> You are reviewing the proposed merge of
> ~mpontillo/maas:add-deploy-kvm-ui--bug-1795013 into maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.