Re: [PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName

2016-06-10 Thread 'Iustin Pop' via ganeti-devel 
On Fri, Jun 10, 2016 at 11:28:16AM +0100, Brian Foley wrote:
> On Thu, Jun 09, 2016 at 11:13:54PM +0200, Iustin Pop wrote:
> > From: Iustin Pop 
> > 
> > This function (and getNode, next patch) were two pain points when I tried to
> > convert UUIDs to another type, since they're called via a single type
> > (currently String) that wants to dispatch to either UUID or name. In many 
> > cases
> > we know exactly what we want to search for, so having only this function is
> > overly generic. Not useful yet, but will be later for the string type
> > conversion.
> > 
> > Additionally change the name of the existing function getInstanceByName to
> > match the new functions better.
> > 
> > Signed-off-by: Iustin Pop 
> 
> It took me a little while to convince myself that some of the callers should
> use getInstanceByExactName, but I think you're correct.

Note that I don't change that fact; I just renamed getInstanceByName to
getInstanceByExactName to make a clear distinction between PartialName
and ExactName.

But yes, it's a good point if they need to do that.

thanks,
iustin

Re: [PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName

2016-06-10 Thread 'Brian Foley' via ganeti-devel 
On Thu, Jun 09, 2016 at 11:13:54PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> This function (and getNode, next patch) were two pain points when I tried to
> convert UUIDs to another type, since they're called via a single type
> (currently String) that wants to dispatch to either UUID or name. In many 
> cases
> we know exactly what we want to search for, so having only this function is
> overly generic. Not useful yet, but will be later for the string type
> conversion.
> 
> Additionally change the name of the existing function getInstanceByName to
> match the new functions better.
> 
> Signed-off-by: Iustin Pop 

It took me a little while to convince myself that some of the callers should
use getInstanceByExactName, but I think you're correct.

LGTM.

> ---
>  src/Ganeti/Config.hs | 43 ++-
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
> index bce7ae3..0d935c4 100644
> --- a/src/Ganeti/Config.hs
> +++ b/src/Ganeti/Config.hs
> @@ -275,22 +275,24 @@ getNode cfg name =
>(nodeName . (M.!) nodes) nodes
>  in getItem "Node" name by_name
>  
> --- | Looks up an instance by name or uuid.
> -getInstance :: ConfigData -> String -> ErrorResult Instance
> -getInstance cfg name =
> +-- | Looks up an instance by uuid.
> +getInstanceByUuid :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByUuid cfg uuid =
>let instances = fromContainer (configInstances cfg)
> -  in case getItem' "Instance" name instances of
> -   -- if not found by uuid, we need to look it up by name
> -   Ok inst -> Ok inst
> -   Bad _ -> let by_name =
> -  M.delete ""
> -  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
> -  $ instances
> -in getItem "Instance" name by_name
> -
> --- | Looks up an instance by exact name match
> -getInstanceByName :: ConfigData -> String -> ErrorResult Instance
> -getInstanceByName cfg name =
> +  in getItem' "Instance" uuid instances
> +
> +-- | Looks up an instance by approximate name.
> +getInstanceByPartialName :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByPartialName cfg name =
> +  let instances = fromContainer (configInstances cfg)
> +  by_name = M.delete ""
> +  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
> +  $ instances
> +  in getItem "Instance" name by_name
> +
> +-- | Looks up an instance by exact name match.
> +getInstanceByExactName :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByExactName cfg name =
>let instances = M.elems . fromContainer . configInstances $ cfg
>matching = F.find (maybe False (== name) . instName) instances
>in case matching of
> @@ -299,6 +301,13 @@ getInstanceByName cfg name =
> ("Instance name " ++ name ++ " not found")
> ECodeNoEnt
>  
> +-- | Looks up an instance by partial name or uuid.
> +getInstance :: ConfigData -> String -> ErrorResult Instance
> +getInstance cfg name =
> +  case getInstanceByUuid cfg name of
> +x@(Ok _) -> x
> +Bad _ -> getInstanceByPartialName cfg name
> +
>  -- | Looks up a disk by uuid.
>  getDisk :: ConfigData -> String -> ErrorResult Disk
>  getDisk cfg name =
> @@ -432,7 +441,7 @@ getFilledInstOsParams cfg inst =
>  -- | Looks up an instance's primary node.
>  getInstPrimaryNode :: ConfigData -> String -> ErrorResult Node
>  getInstPrimaryNode cfg name =
> -  getInstanceByName cfg name
> +  getInstanceByExactName cfg name
>>>= withMissingParam "Instance without primary node" return . 
> instPrimaryNode
>>>= getNode cfg
>  
> @@ -451,7 +460,7 @@ getDrbdDiskNodes cfg disk =
>  -- the primary node has to be appended to the results.
>  getInstAllNodes :: ConfigData -> String -> ErrorResult [Node]
>  getInstAllNodes cfg name = do
> -  inst <- getInstanceByName cfg name
> +  inst <- getInstanceByExactName cfg name
>inst_disks <- getInstDisksFromObj cfg inst
>let disk_nodes = concatMap (getDrbdDiskNodes cfg) inst_disks
>pNode <- getInstPrimaryNode cfg name
> -- 
> 2.8.1
>

[PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName

2016-06-09 Thread Iustin Pop 
From: Iustin Pop 

This function (and getNode, next patch) were two pain points when I tried to
convert UUIDs to another type, since they're called via a single type
(currently String) that wants to dispatch to either UUID or name. In many cases
we know exactly what we want to search for, so having only this function is
overly generic. Not useful yet, but will be later for the string type
conversion.

Additionally change the name of the existing function getInstanceByName to
match the new functions better.

Signed-off-by: Iustin Pop 
---
 src/Ganeti/Config.hs | 43 ++-
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
index bce7ae3..0d935c4 100644
--- a/src/Ganeti/Config.hs
+++ b/src/Ganeti/Config.hs
@@ -275,22 +275,24 @@ getNode cfg name =
   (nodeName . (M.!) nodes) nodes
 in getItem "Node" name by_name
 
--- | Looks up an instance by name or uuid.
-getInstance :: ConfigData -> String -> ErrorResult Instance
-getInstance cfg name =
+-- | Looks up an instance by uuid.
+getInstanceByUuid :: ConfigData -> String -> ErrorResult Instance
+getInstanceByUuid cfg uuid =
   let instances = fromContainer (configInstances cfg)
-  in case getItem' "Instance" name instances of
-   -- if not found by uuid, we need to look it up by name
-   Ok inst -> Ok inst
-   Bad _ -> let by_name =
-  M.delete ""
-  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
-  $ instances
-in getItem "Instance" name by_name
-
--- | Looks up an instance by exact name match
-getInstanceByName :: ConfigData -> String -> ErrorResult Instance
-getInstanceByName cfg name =
+  in getItem' "Instance" uuid instances
+
+-- | Looks up an instance by approximate name.
+getInstanceByPartialName :: ConfigData -> String -> ErrorResult Instance
+getInstanceByPartialName cfg name =
+  let instances = fromContainer (configInstances cfg)
+  by_name = M.delete ""
+  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
+  $ instances
+  in getItem "Instance" name by_name
+
+-- | Looks up an instance by exact name match.
+getInstanceByExactName :: ConfigData -> String -> ErrorResult Instance
+getInstanceByExactName cfg name =
   let instances = M.elems . fromContainer . configInstances $ cfg
   matching = F.find (maybe False (== name) . instName) instances
   in case matching of
@@ -299,6 +301,13 @@ getInstanceByName cfg name =
("Instance name " ++ name ++ " not found")
ECodeNoEnt
 
+-- | Looks up an instance by partial name or uuid.
+getInstance :: ConfigData -> String -> ErrorResult Instance
+getInstance cfg name =
+  case getInstanceByUuid cfg name of
+x@(Ok _) -> x
+Bad _ -> getInstanceByPartialName cfg name
+
 -- | Looks up a disk by uuid.
 getDisk :: ConfigData -> String -> ErrorResult Disk
 getDisk cfg name =
@@ -432,7 +441,7 @@ getFilledInstOsParams cfg inst =
 -- | Looks up an instance's primary node.
 getInstPrimaryNode :: ConfigData -> String -> ErrorResult Node
 getInstPrimaryNode cfg name =
-  getInstanceByName cfg name
+  getInstanceByExactName cfg name
   >>= withMissingParam "Instance without primary node" return . instPrimaryNode
   >>= getNode cfg
 
@@ -451,7 +460,7 @@ getDrbdDiskNodes cfg disk =
 -- the primary node has to be appended to the results.
 getInstAllNodes :: ConfigData -> String -> ErrorResult [Node]
 getInstAllNodes cfg name = do
-  inst <- getInstanceByName cfg name
+  inst <- getInstanceByExactName cfg name
   inst_disks <- getInstDisksFromObj cfg inst
   let disk_nodes = concatMap (getDrbdDiskNodes cfg) inst_disks
   pNode <- getInstPrimaryNode cfg name
-- 
2.8.1