On Wed, Jun 22, 2016 at 06:34:41PM +0100, 'Viktor Bachraty' via ganeti-devel wrote: > This patch fixes the bug that came from the different representation on > data Group in HTools compared to NodeGroup in Objects. The first one > uses only a list of network IDs, while the latter uses a container of > PartialNicParams.
Great. LGTM apart from a two minor inline nits. Cheers, Brian. > Signed-off-by: Viktor Bachraty <[email protected]> > --- > src/Ganeti/HTools/Backend/Luxi.hs | 5 +++-- > src/Ganeti/JSON.hs | 7 +++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/Ganeti/HTools/Backend/Luxi.hs > b/src/Ganeti/HTools/Backend/Luxi.hs > index c4c8078..11fe2a8 100644 > --- a/src/Ganeti/HTools/Backend/Luxi.hs > +++ b/src/Ganeti/HTools/Backend/Luxi.hs > @@ -52,6 +52,7 @@ import qualified Ganeti.HTools.Group as Group > import qualified Ganeti.HTools.Node as Node > import qualified Ganeti.HTools.Instance as Instance > import Ganeti.JSON > +import Ganeti.Objects as O > > {-# ANN module "HLint: ignore Eta reduce" #-} > > @@ -252,8 +253,8 @@ parseGroup [uuid, name, apol, ipol, tags, nets] = do > xapol <- convert "alloc_policy" apol > xipol <- convert "ipolicy" ipol > xtags <- convert "tags" tags > - xnets <- convert "networks" nets > - return (xuuid, Group.create xname xuuid xapol xnets xipol xtags) > + xnets <- convert "networks" nets :: Result (Container PartialNicParams) > + return (xuuid, Group.create xname xuuid xapol (getKeysFromContainer xnets) > xipol xtags) Nit: this is >80 chars, could you split or move the getKeysFromContainer into a separate let expression? > parseGroup v = fail ("Invalid group query result: " ++ show v) > > diff --git a/src/Ganeti/JSON.hs b/src/Ganeti/JSON.hs > index 0248d6a..cb88162 100644 > --- a/src/Ganeti/JSON.hs > +++ b/src/Ganeti/JSON.hs > @@ -62,6 +62,7 @@ module Ganeti.JSON > , lookupContainer > , alterContainerL > , readContainer > + , getKeysFromContainer > , mkUsedKeys > , allUsedKeys > , DictObject(..) > @@ -338,6 +339,12 @@ emptyContainer = GenericContainer Map.empty > -- | Type alias for string keys. > type Container = GenericContainer BS.ByteString > > +-- | Returns all string keys from a container. > +getKeysFromContainer :: (Container a) -> [String] > +getKeysFromContainer c = > + let m = fromContainer c in > + map UTF8.toString $ Map.keys m > + I get to play 'point-free style police' here. This might read better as getKeysFromContainer = map UTF8.toString . Map.keys . fromContainer (as I had to remind myself a few times, the . operator is right associative and high precedence, so this is equivalent to ((map UTF8.toString) . Map.keys) . fromContainer) > instance HasStringRepr BS.ByteString where > fromStringRepr = return . UTF8.fromString > toStringRepr = UTF8.toString > -- > 2.8.0.rc3.226.g39d4020 >
