Re: [Engine-devel] "wipe after delete" Matching defaults from GUI to Backend

On 10/11/2012 05:28 PM, Daniel Erez wrote: > > > ----- Original Message ----- >> From: "Ravi Nori" <rnori(a)redhat.com&gt; >> To: "Daniel Erez" <derez(a)redhat.com&gt; >> Cc: engine-devel(a)ovirt.org >> Sent: Thursday, October 11, 2012 3:21:05 PM >> Subject: Re: [Engine-devel] "wipe after delete" Matching defaults >> from GUI to Backend >> >> On 10/10/2012 01:17 AM, Daniel Erez wrote: >>> >>> ----- Original Message ----- >>>> From: "Ravi Nori" <rnori(a)redhat.com&gt; >>>> To: engine-devel(a)ovirt.org >>>> Sent: Tuesday, October 9, 2012 6:40:56 PM >>>> Subject: [Engine-devel] "wipe after delete" Matching defaults from >>>> GUI to Backend >>>> >>>> I am working on the bug where the defaults values in the GUI for >>>> disk >>>> actions don't correspond to the default values in the API/Backend. >>>> For >>>> example the "wipe after delete" flag defaults to true in the GUI >>>> but >>>> false in the backend code. Default values are not sent from the >>>> frontend >>>> to the backend. Please refer to the bugzilla link >>>> >>>> https://bugzilla.redhat.com/show_bug.cgi?id=845466 >>>> >>>> After discussing with Michael as to how to approach fixing this, >>>> we >>>> think the best way to do it would be to fix it in the Backend >>>> class >>>> businessentities.Disk by replacing primitives with reference types >>>> and >>>> forcing business logic in the class to set the default values to >>>> match >>>> the default values in the client. >>>> >>>> I would appreciate some feedback on this and also on what the >>>> default >>>> values are for Disk operations on the GUI. >>> GUI behavior is as follows: >>> For File storage domains - WipeAfterDelete check-box is >>> unchangeable and unchecked (false). >>> For Block storage domains - WipeAfterDelete check-box is changeable >>> and >>> the default value is according to 'SANWipeAfterDelete' >>> configuration value. >>> >>> I think you should simply utilize the same configuration value in >>> REST or backend when creating a new disk. >>> >>>> Thanks >>>> >>>> Ravi >>>> _______________________________________________ >>>> Engine-devel mailing list >>>> Engine-devel(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>> >> Thanks for the reply. >> >> Here is my current implementation >> >> I modified businessentities.BaseDisk class to change wipeAfterDelete >> to >> wrapper class (reference class) for boolean >> >> private Boolean wipeAfterDelete; >> >> I also added default values for File and block storages >> private boolean wipeAfterDeleteFileStorageDomain = false; >> private boolean wipeAfterDeleteBlockStorageDomain = >> Config.<Boolean>GetValue(ConfigValues.SANWipeAfterDelete); >> >> The storage type determines which of the two default values is >> returned >> by BaseDisk when wipeAfterDelete is not set (is null) >> StorageType storageType = StorageType.UNKNOWN; >> >> So isWipeAfterDeleteMethodLooks like this >> >> public boolean isWipeAfterDelete() { >> if (wipeAfterDelete == null) { >> return getDefaultIsWipeAfterDelete(); >> } >> return wipeAfterDelete; >> } >> >> private boolean getDefaultIsWipeAfterDelete() { >> if(storageType.equals(StorageType.UNKNOWN)) { >> return false; >> } >> switch (storageType) { >> case ISCSI: >> case FCP: >> return wipeAfterDeleteBlockStorageDomain; >> default: >> return wipeAfterDeleteFileStorageDomain; >> } >> } >> >> There is a new method isWipeAfterDeleteSet that will be used by >> commands >> to set the storage type to get the default wipeAfterDeleteFlag if >> wipeAfterDelete was not set. >> >> public boolean isWipeAfterDeleteSet() { >> return wipeAfterDelete != null; >> } >> >> Now in bll.AddDiskCommand I modified executeVmCommand to add the >> following >> >> if(!getParameters().getDiskInfo().isWipeAfterDeleteSet()) { >> StorageType storageType = >> getStorageDomain().getstorage_type(); >> getParameters().getDiskInfo().setStorageType(storageType); >> } >> >> This approach minimizes the impact of changes. So all commands >> interested in wipeAfterDelete flag based on storage type if >> wipeAfterDelete has not been set need to set the storageType before >> calling isWipeAfterDelete. >> >> Please let me know if this is the solution conflicts with something >> you >> have been working on. All the existing code will work as it has been >> working, if wipeAfterDelete has not been set and storageType has not >> been set the method isWipeAfterDelete return false (like it was >> earlier) >> > > > Calling 'Config.<?> GetValue' in a business entity could be > problematic in the UI since > only the backend can invoke it (business entities are common for the > backend and the UI > which means that both may instantiate/manipulate them). > We use GWT RPC for serializing/deserializing these business entities, > thus we usually can't reference non plain java objects (or use some > tweaks/tricks if applicable). > Try to check the UI behavior after the new changes and let me know... > [as an alternative, you can probably get 'SANWipeAfterDelete' config > value from REST code (or backend)]. clients (UI/API) can only call the ConfigurationValues enum, and should use the query for it (gui has this already of course, just look it up)