Alissa Bonas has posted comments on this change.
Change subject: [wip] providers: add the openstack image support
......................................................................
Patch Set 4: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/OpenstackImageProviderProxy.java
Line 24:
Line 25:
Line 26: public class OpenstackImageProviderProxy implements ProviderProxy {
Line 27:
Line 28: enum GlanceImageFormat {
Why not use existing enum VolumeFormat?
Line 29: RAW("raw"),
Line 30: ISO("iso"),
Line 31: COW("qcow2");
Line 32:
Line 41: }
Line 42: }
Line 43:
Line 44: enum GlanceImageContainer {
Line 45: BARE("bare");
Why is there a need in enum for a single value?
Line 46:
Line 47: private String value;
Line 48:
Line 49: GlanceImageContainer(String value) {
Line 80: return DbFacade.getInstance();
Line 81: }
Line 82:
Line 83: @Override
Line 84: public void onAddition() {
Where/how the user can perform this action? (UI? REST?)
And will those glance domains displayed in webadmin along with all the rest of
storage domains?
Line 85: // Storage domain static
Line 86: StorageDomainStatic domainStaticEntry = new
StorageDomainStatic();
Line 87: domainStaticEntry.setId(Guid.newGuid());
Line 88: domainStaticEntry.setStorage(provider.getId().toString());
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/provider/ProviderDaoDbFacadeImpl.java
Line 39: tenantName = networkProperties.getTenantName();
Line 40: pluginType = networkProperties.getPluginType().name();
Line 41: break;
Line 42: case OPENSTACK_IMAGE:
Line 43: OpenstackImageProviderProperties imageProperties =
if both OpenstackNetworkProviderProperties and OpenstackImageProviderProperties
have the same property tenantName - perhaps it's better to create a base class
that contains this property.
It will be also more clean to set it once in the mapAdditionalProperties method
below.
Line 44: (OpenstackImageProviderProperties)
entity.getAdditionalProperties();
Line 45: tenantName = imageProperties.getTenantName();
Line 46: break;
Line 47: default:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainStaticDAO.java
Line 24: * @param storage
Line 25: * the storage value
Line 26: * @return the domain
Line 27: */
Line 28: List<StorageDomainStatic> getAllByStorage(String storage);
What the storage string represents? the glance provider? From which db
table/entity it's taken?
As Allon also mentioned, a similar method exists in StorageDomainDao - public
Any reason not to use it instead of adding new procedure/dao methods?
Line 29:
Line 30: /**
Line 31: * Retrieves all domains of the specified type for the specified
pool.
Line 32: *
--
To view, visit http://gerrit.ovirt.org/15899
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I424998de6f6514d65938484fffa5405e35857e43
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
