Mike Kolesnik has posted comments on this change.

Change subject: core: Add mom policy update command
......................................................................


Patch Set 10:

(3 comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMomPolicyCommand.java
Line 43:                 && validate(validateMinimumVersionSupport());
Line 44:     }
Line 45: 
Line 46:     private ValidationResult validateMinimumVersionSupport() {
Line 47:         return FeatureSupported.momPolicyOnHost(Version.v3_3)
What you should be sending is the cluster compatibility version, not a constant 
version..

You could send the host's version but that would be confusing to the users. The 
norm in oVirt is to restrict version specific features by either cluster or DC 
- since this is clearly related to cluster (and not related to storage), the 
restriction should be by cluster level.
Line 48:                 ? ValidationResult.VALID
Line 49:                 : new 
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_VERSION);
Line 50:     }
Line 51: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsValidator.java
Line 83:     }
Line 84: 
Line 85:     public ValidationResult validateStatus(VDSStatus vdsStatus, 
VdcBllMessages hostStatus) {
Line 86:         ValidationResult existsValidation = exists();
Line 87:         if (!existsValidation.isValid()) {
I haven't seen a response from you regarding the comment on patch set 5.

I still think this check shouldn't be here.. My reasoning is in patch set 5
Line 88:             return existsValidation;
Line 89:         }
Line 90: 
Line 91:         return vdsStatus == vds.getStatus()


....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 527: ACTION_TYPE_FAILED_VDS_INTERMITENT_CONNECTIVITY=Due to intermittent 
connectivity to this Host, fence operations are not allowed at this time. The 
system is trying to reconnect, please try again in 30 seconds.
Line 528: ACTION_TYPE_FAILED_PM_ENABLED_WITHOUT_AGENT=Cannot ${action} ${type}. 
Power Management is enabled for Host but no Agent type selected.
Line 529: ACTION_TYPE_FAILED_PM_ENABLED_WITHOUT_AGENT_CREDENTIALS=Cannot 
${action} ${type}. Power Management is enabled for Host but Agent credentials 
are missing.
Line 530: ACTION_TYPE_FAILED_AGENT_NOT_SUPPORTED=Cannot ${action} ${type}. 
Selected Power Management Agent is not supported.
Line 531: ACTION_TYPE_FAILED_VDS_VERSION=Cannot ${action} ${type}. Feature is 
not supported in host version.
This message is not very intuitive to user - what feature? what version? how 
can he fix this?

Besides it should refer to the cluster's version, and be named appropriately: 
ACTION_TYPE_FAILED_CLUSTER_VERSION_NOT_SUPPORTED

A proposed alternative (if you aim at a generic message):
Cannot ${action} ${type}. The cluster's compatibility version doesn't support 
this action. Please update the cluster's compatibility version and try again.

However, usually its best to have a specific message that is more 
understandable and intuitive to the users, such as "Updating the host's policy 
is not supported for the cluster's compatibility version, etc.."
Line 532: NETWORK_BOND_NOT_ATTACCH_TO_NETWORK=Bond is not attached to Network.
Line 533: NETWORK_INTERFACE_NOT_ATTACCH_TO_NETWORK=Network Interface is not 
attached to Logical Network.
Line 534: NETWORK_INTERFACE_IN_USE_BY_VLAN=Bonding cannot be applied on an 
Interface where VLAN is defined.\n\
Line 535:       -Please remove VLAN from the interface.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64350c17f182c1dd0ba1d06130e5d56019b32fc9
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <oma...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Martin Sivák <msi...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to