Omer Frenkel has posted comments on this change.
Change subject: engine: Create Gluster Volume & Volume Option Set
......................................................................
Patch Set 2: (12 inline comments)
....................................................
File backend/manager/dbscripts/create_functions.sql
Line 254: SELECT cluster_id AS id
missing also data-center
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeCommand.java
Line 37: super(params);
since permission is required on cluster, the cluster should be initialized in
the constructor, so i suggest moving this line from the canDoAction here:
setVdsGroupId(volume.getClusterId())
(of course add a null check on volume before)
that way, if volume and cluster supplied, permission will be checked on it, if
not, user will not be authorized to run the action
Line 93: setGlusterVolumeName(volume.getName());
now that the gluster volume is saved to the db we can change the
getGlusterVolumeName in auditLogableBase to get the value from the db if its
null, instead of requiring the user to set this when he likes to
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeCommandBase.java
Line 23: setVdsGroupId(getGlusterVolume().getClusterId());
i didnt see where getGlusterVolume is defined, but are we sure it can't return
null?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/SetGlusterVolumeOptionCommand.java
Line 57: setGlusterVolumeId(option.getVolumeId());
the above is redundant, as the setGlusterVolumeId was already called in the
super command's ctor
for the below see comment in previous command
Line 62: getGlusterVolumeName(),
getParameters().getVolumeOption()));
why do we need to pass this if we save the option as a field in the command?
Line 80: if(getGlusterVolume().getOptionValue(option.getKey())
!= null) {
how can this be null? isn't there a check in the canDoAction to block this from
being null?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/CreateGlusterVolumeParameters.java
Line 15: super();
no need to explicit call the super()
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java
Line 541: public String getGlusterVolumeName() {
here can be added a check if glusterVolumeName is null, then check if
getGlusterVolume() != null and return getGlusterVolume().getName()
this way user doesn't need to set id AND name all the time, id will be enough
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 664: VAR__TYPE__GLUSTER_VOLUME=${type} Gluster Volume
missing VAR__TYPE__GLUSTER_VOLUME_OPTION
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 494: GLUSTER_VOLUME_CREATE_FAILED=Creation of Gluster Volume
${glusterVolumeName} failed.
missing GLUSTER_VOLUME_OPTION_SET and GLUSTER_VOLUME_OPTION_SET_FAILED
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/CreateGlusterVolumeVDSCommand.java
Line 38: parameters.put("volumeType",
volume.getVolumeType().toString());
i think volume.getVolumeType().name() should be used for enums
--
To view, visit http://gerrit.ovirt.org/3142
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e5a857d188450409ce9f6eefd4df6bc1b10f9b2
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
