Michael Pasternak has posted comments on this change.
Change subject: API: host-deploy: Adding ssh username, port and fingerprint to
host information
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(14 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1548: <xs:extension base="BaseResource">
Line 1549: <xs:sequence>
Line 1550: <xs:element name="port" type="xs:int" minOccurs="0"
maxOccurs="1"/>
Line 1551: <xs:element name="fingerprint" type="xs:string"
minOccurs="0" maxOccurs="1"/>
Line 1552: <xs:element ref="authentication_type" minOccurs="0"
maxOccurs="1"/>
not sure why it should be expressed via dedicated type [1], we used to expose
all enums via standard string element.
[1]
<host>
<ssh>
<authentication_type>
<authentication_type>
Line 1553: <xs:element ref="user" minOccurs="0" maxOccurs="1"/>
Line 1554: </xs:sequence>
Line 1555: </xs:extension>
Line 1556: </xs:complexContent>
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 1847: body:
Line 1848: parameterType: Host
Line 1849: signatures:
Line 1850: - mandatoryArguments: {}
Line 1851: optionalArguments: {host.name: 'xs:string', host.address:
'xs:string', host.root_password: 'xs:string', host.ssh.port: 'xs:int',
host.ssh.fingerprint: 'xs:string', host.ssh.authentication: 'xs:int',
what this "host.ssh.authentication: 'xs:int'" stands for? authentication_type?
then it should be string
Line 1852: host.ssh.password: 'xs:string', host.display.address:
'xs:string', host.cluster.id|name: 'xs:string',
Line 1853: host.port: 'xs:int', host.storage_manager.priority:
'xs:int', host.power_management.type: 'xs:string',
Line 1854: host.power_management.enabled: 'xs:boolean',
host.power_management.address: 'xs:string', host.power_management.username:
'xs:string',
Line 1855: host.power_management.password: 'xs:string',
host.power_management.options.option--COLLECTION: {option.name: 'xs:string',
option.value: 'xs:string'}, host.power_management.pm_proxy--COLLECTION:
{propietary : 'xs:string'},
host.power_management.agents.agent--COLLECTION:{type: 'xs:string', address:
'xs:string', user_name: 'xs:string', password: 'xs:string',
options.option--COLLECTION: {option.name: 'xs:string', option.value:
'xs:string'}}}
Line 1863: body:
Line 1864: parameterType: Host
Line 1865: signatures:
Line 1866: - mandatoryArguments: {host.name: 'xs:string', host.address:
'xs:string', host.root_password: 'xs:string', host.cluster.id|name: 'xs:string'}
Line 1867: optionalArguments: {host.ssh.port: 'xs:int',
host.ssh.fingerprint: 'xs:string', host.ssh.authentication: 'xs:int',
host.ssh.password: 'xs:string', host.port: 'xs:int',
same
Line 1868: host.display.address: 'xs:string',
host.storage_manager.priority: 'xs:int', host.power_management.type:
'xs:string',
Line 1869: host.power_management.enabled: 'xs:boolean',
host.power_management.address: 'xs:string', host.power_management.username:
'xs:string',
Line 1870: host.power_management.password: 'xs:string',
host.power_management.options.option--COLLECTION: {option.name: 'xs:string',
option.value: 'xs:string'}, host.power_management.pm_proxy--COLLECTION:
{propietary : 'xs:string'},
host.power_management.agents.agent--COLLECTION:{type: 'xs:string', address:
'xs:string', user_name: 'xs:string', password: 'xs:string',
options.option--COLLECTION: {option.name: 'xs:string', option.value:
'xs:string'}}, host.reboot_after_installation: 'xs:boolean'}
Line 1871: urlparams: {}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostResource.java
Line 144: public Response approve(Action action) {
Line 145:
Line 146: if (action.isSetCluster() && (action.getCluster().isSetId()
|| action.getCluster().isSetName())) {
Line 147: update(setCluster(get(), action.getCluster()));
Line 148: }
implement dedicated validateEnums(Action.class, action); it will validate
AuthenticationType for you (see validateEnums(VM.class, vm); for details)
Line 149: ApproveVdsParameters params = new ApproveVdsParameters(guid);
Line 150: params.setPassword(action.getRootPassword());
Line 151: if (action.isSetSsh()) {
Line 152: if (action.getSsh().isSetUser()) {
Line 156: }
Line 157: if (action.getSsh().isSetAuthenticationType()) {
Line 158:
params.setAuthMethod(mapAuth(action.getSsh().getAuthenticationType().getAuthenticationType()));
Line 159: }
Line 160: }
i'd move this logic to new HostMaper.map(Action model, ApproveVdsParameters
template)
mapper
Line 161: return doAction(VdcActionType.ApproveVds,
Line 162: params,
Line 163: action);
Line 164: }
Line 394: public BackendHostHooksResource getHooksResource() {
Line 395: return inject(new BackendHostHooksResource(id));
Line 396: }
Line 397:
Line 398: private
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod
mapAuth(String type) {
1. naming convention is map()
2. should be moved to the HostMapper
Line 399: AuthenticationMethod authMethod =
AuthenticationMethod.fromValue(type);
Line 400: return getMapper(AuthenticationMethod.class,
Line 401:
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod.class).map(authMethod,
Line 402: null);
Line 395: return inject(new BackendHostHooksResource(id));
Line 396: }
Line 397:
Line 398: private
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod
mapAuth(String type) {
Line 399: AuthenticationMethod authMethod =
AuthenticationMethod.fromValue(type);
what if authMethod==NULL ?... NPE?
Line 400: return getMapper(AuthenticationMethod.class,
Line 401:
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod.class).map(authMethod,
Line 402: null);
Line 403: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostsResource.java
Line 85: }
Line 86:
Line 87: @Override
Line 88: public Response add(Host host) {
Line 89: validateEnums(Host.class, host);
make validateEnums(Host.class, host); to validate AuthenticationType for you
Line 90: validateParameters(host, "name", "address", "rootPassword");
Line 91: VdsStatic staticHost = getMapper(Host.class,
VdsStatic.class).map(host, null);
Line 92: staticHost.setVdsGroupId(getClusterId(host));
Line 93: AddVdsActionParameters addParams = new
AddVdsActionParameters(staticHost, host.getRootPassword());
Line 207: ? host.getCluster().getName()
Line 208: : "Default")).getId();
Line 209: }
Line 210:
Line 211: private
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod
mapAuth(String type) {
1. naming convention is map()
2. should be moved to the HostMapper
Line 212: AuthenticationMethod authMethod =
AuthenticationMethod.fromValue(type);
Line 213: return getMapper(AuthenticationMethod.class,
Line 214:
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod.class).map(authMethod,
Line 215: null);
Line 208: : "Default")).getId();
Line 209: }
Line 210:
Line 211: private
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod
mapAuth(String type) {
Line 212: AuthenticationMethod authMethod =
AuthenticationMethod.fromValue(type);
what if authMethod==NULL ?... NPE?
Line 213: return getMapper(AuthenticationMethod.class,
Line 214:
org.ovirt.engine.core.common.action.VdsOperationActionParameters.AuthenticationMethod.class).map(authMethod,
Line 215: null);
Line 216: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostResourceTest.java
Line 668: @Override
Line 669: protected VDS getEntity(int index) {
Line 670: VdsStatic vdsStatic = control.createMock(VdsStatic.class);
Line 671: expect(vdsStatic.getId()).andReturn(GUIDS[index]).anyTimes();
Line 672: VDS entity =
setUpEntityExpectations(control.createMock(VDS.class), null, vdsStatic, index);
did you checked that this change is not break other tests?
Line 673:
expect(entity.getStoragePoolId()).andReturn(GUIDS[1]).anyTimes();
Line 674: return entity;
Line 675: }
Line 676:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostsResourceTest.java
Line 301: protected VDS getEntity(int index) {
Line 302: return setUpEntityExpectations(control.createMock(VDS.class),
Line 303:
control.createMock(VdsStatistics.class),
Line 304:
control.createMock(VdsStatic.class),
Line 305: index);
did you checked that this change is not breaks other tests?
Line 306: }
Line 307:
Line 308: static VDS setUpEntityExpectations(VDS entity, int index) {
Line 309: return setUpEntityExpectations(entity, null, null, index);
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendResourceTest.java
Line 154:
Line 155: protected VDS getEntity(int index) {
Line 156: VdsStatic vdsStatic = control.createMock(VdsStatic.class);
Line 157: expect(vdsStatic.getId()).andReturn(GUIDS[2]).anyTimes();
Line 158: VDS vds =
setUpEntityExpectations(control.createMock(VDS.class), null, vdsStatic, index);
did you checked that this change is not breaks other tests?
Line 159: return vds;
Line 160: }
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/HostMapperTest.java
Line 230:
Line 231: VdsStatic mappedVdsStatic = HostMapper.map(sshConf,
vdsStatic);
Line 232: assertEquals(mappedVdsStatic.getSshPort(), 22);
Line 233: assertEquals(mappedVdsStatic.getSshKeyFingerprint(), "1234");
Line 234: assertEquals(mappedVdsStatic.getSshUsername(), "root");
no test for AuthType?
Line 235: }
Line 236:
Line 237: @Test
Line 238: public void testUpdatePowerManagementHostFromAgents() {
--
To view, visit http://gerrit.ovirt.org/16685
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic18e91f1602af42e7c73acea5d875c54545cb3c2
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
