Vitor de Lima has posted comments on this change.

Change subject: core, webadmin, restapi: Fix VM creation with blank template
......................................................................


Patch Set 8:

(2 comments)

....................................................
File 
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmsResource.java
Line 108: 
Line 109:                 VDSGroup cluster = 
lookupCluster(staticVm.getVdsGroupId());
Line 110:                 if (cluster.getArchitecture() == 
ArchitectureType.undefined) {
Line 111:                     throw new 
WebApplicationException(Response.status(Response.Status.BAD_REQUEST)
Line 112:                             .entity("The selected cluster does not 
have architecture, please set it before adding vms.")
What do you think about dropping this validation and for the cases where the 
target cluster have an undefined architecture we let the canDo of the 
AddVmCommand fail properly? (This will happen due to the recent merge of the 
change  #18227)
Line 113:                             .build());
Line 114:                 }
Line 115: 
Line 116:                 if (Guid.Empty.equals(templateId) && !vm.isSetOs()) {


Line 155: 
Line 156:         if (defaultOs == null) {
Line 157:             throw new 
WebApplicationException(Response.status(Response.Status.BAD_REQUEST)
Line 158:                     .entity("Could not determine the default OS based 
on the architecture of the selected cluster.")
Line 159:                     .build());
I thought about replacing this method with:

private void replaceDefaultOs(VmStatic vm, VDSGroup cluster)
{
    OsRepository osRepository = 
SimpleDependecyInjector.getInstance().get(OsRepository.class);
                
    if (cluster.getArchitecture() != ArchitectureType.undefined) {
        Integer defaultOs = 
osRepository.getDefaultOSes().get(cluster.getArchitecture());

        vm.setOsId(defaultOs);
    }
}

Because it doesn't matter which OS we pass to the command, it will fail anyway 
if the architecture is undefined.
Line 160:         }
Line 161: 
Line 162:         vm.setOsId(defaultOs);
Line 163:     }


-- 
To view, visit http://gerrit.ovirt.org/20667
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I880e5940c7244476e477e098445a47210ea08b5d
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vitor de Lima <[email protected]>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa 
<[email protected]>
Gerrit-Reviewer: Leonardo Bianconi <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Vitor de Lima <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to