Michael Pasternak has posted comments on this change.
Change subject: API: Adding ssh username, port and fingerprint to host
information
......................................................................
Patch Set 12: I would prefer that you didn't submit this
(7 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 1796: parameterType: Host
Line 1797: signatures:
Line 1798: - mandatoryArguments: {}
Line 1799: optionalArguments: {host.name: 'xs:string', host.address:
'xs:string', host.root_password: 'xs:string', host.ssh.port: 'xs:int',
host.ssh.fingerprint: 'xs:string',
Line 1800: host.ssh.username: 'xs:string', host.display.address:
'xs:string', host.cluster.id|name: 'xs:string',
did you meant host.ssh.password here (instead of host.ssh.use
rname) ?
Line 1801: host.port: 'xs:int', host.storage_manager.priority:
'xs:int', host.power_management.type: 'xs:string',
Line 1802: host.power_management.enabled: 'xs:boolean',
host.power_management.address: 'xs:string', host.power_management.username:
'xs:string',
Line 1803: 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 1804: urlparams:
Line 1811: body:
Line 1812: parameterType: Host
Line 1813: signatures:
Line 1814: - mandatoryArguments: {host.name: 'xs:string', host.address:
'xs:string', host.root_password: 'xs:string', host.cluster.id|name: 'xs:string'}
Line 1815: optionalArguments: {host.ssh.port: 'xs:int',
host.ssh.fingerprint: 'xs:string', host.ssh.username: 'xs:string',ost.port:
'xs:int',
1. s/ots/host ?
2. i see you already defined the 'host.ssh.port' in the beginning of optional
params
3. did you meant host.ssh.password here (instead of host.ssh.use
rname) ?
Line 1816: host.display.address: 'xs:string',
host.storage_manager.priority: 'xs:int', host.power_management.type:
'xs:string',
Line 1817: host.power_management.enabled: 'xs:boolean',
host.power_management.address: 'xs:string', host.power_management.username:
'xs:string',
Line 1818: 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 1819: urlparams: {}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostsResource.java
Line 112: } else {
Line 113: // If ssh port was not set, use default.
Line 114: if (addParams.getvds().getSshPort() == 0) {
Line 115: addParams.getvds().setSshPort(DEFAULT_SSH_PORT);
Line 116: }
you still set default port in the client while it should be done in the
addparams.port=DEFAULT_PORT
also i see you already not doing this in the BackendHostResource.install()
Line 117: }
Line 118: if (host.getSsh().isSetFingerprint()) {
Line 119:
addParams.getvds().setSshKeyFingerprint(host.getSsh().getFingerprint());
Line 120: }
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/HostMapper.java
Line 78: /* TODO: add when configured ssh username is enabled
Line 79: if (model.isSetSsh() && model.getSsh().isSetUser() &&
model.getSsh().getUser().isSetUserName()) {
Line 80:
entity.setSshUsername(model.getSsh().getUser().getUserName());
Line 81: } else {
Line 82: entity.setSshUsername(DEFAULT_ROOT_USERNAME);
consider moving this logic to the backend entity, you can do it simply by
defining a default values for the username/port variable in the entity,
the grate benefit of this change is "one-place" change when you need to change
the defaults, also this is less error prone for the clients which won't have to
define the defaults by themself
Line 83: }
Line 84: */
Line 85: if (model.isSetSsh() && model.getSsh().isSetPort() &&
model.getSsh().getPort() > 0) {
Line 86: entity.setSshPort(model.getSsh().getPort());
Line 84: */
Line 85: if (model.isSetSsh() && model.getSsh().isSetPort() &&
model.getSsh().getPort() > 0) {
Line 86: entity.setSshPort(model.getSsh().getPort());
Line 87: } else {
Line 88: entity.setSshPort(DEFAULT_SSH_PORT);
same here
Line 89: }
Line 90: if (model.isSetSsh() && model.getSsh().isSetFingerprint()) {
Line 91:
entity.setSshKeyFingerprint(model.getSsh().getFingerprint());
Line 92: }
Line 88: entity.setSshPort(DEFAULT_SSH_PORT);
Line 89: }
Line 90: if (model.isSetSsh() && model.getSsh().isSetFingerprint()) {
Line 91:
entity.setSshKeyFingerprint(model.getSsh().getFingerprint());
Line 92: }
*not must*: you can do all that via dedicated mapper as you did for [1], it
will be just a bit more pretty
[1] map(VdsStatic entity, SSH template)
Line 93: if (model.isSetPowerManagement()) {
Line 94: entity = map(model.getPowerManagement(), entity);
Line 95: }
Line 96: if (model.isSetStorageManager()) {
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/HostMapperTest.java
Line 52: assertNotNull(transform.getCluster());
Line 53: assertEquals(model.getCluster().getId(),
transform.getCluster().getId());
Line 54: assertEquals(model.getAddress(), transform.getAddress());
Line 55: assertEquals(model.getPort(), transform.getPort());
Line 56: assertEquals(model.getSsh(), transform.getSsh());
you didn't wrote equals() method for the SSH object [1], so here compared the
reference, not content,
please compare fields instead.
[1] also you can write it as SSH auto-generated by jaxb
Line 57: assertEquals(model.getStorageManager().getPriority(),
transform.getStorageManager().getPriority());
Line 58: assertEquals(model.getDisplay().getAddress(),
transform.getDisplay().getAddress());
Line 59: }
Line 60:
--
To view, visit http://gerrit.ovirt.org/16247
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b8c117d041c29780af2468888a732ee664d283c
Gerrit-PatchSet: 12
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sahina Bose <[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
