On Wed, Aug 7, 2013 at 8:21 PM, Jose A. Lopes <[email protected]> wrote:
> From: "Jose A. Lopes" <[email protected]> > > * fix some return types in opcodes and some types and documentation in > parameters. > * fix serialization of the default value of the 'name' parameter in > the opcodes related to tags when tag kind is 'cluster' > (Not very serious, but splitting the two (or three, really) patches would have made reviewing a bit easier... But don't worry, leave it as is for now.) > > Signed-off-by: Jose A. Lopes <[email protected]> > --- > lib/cli.py | 2 +- > lib/cmdlib/instance_operation.py | 4 ++-- > src/Ganeti/HTools/Program/Harep.hs | 6 ++---- > src/Ganeti/OpCodes.hs | 6 +++--- > src/Ganeti/OpParams.hs | 27 +++++++++++++++++++-------- > test/hs/Test/Ganeti/OpCodes.hs | 2 +- > test/hs/Test/Ganeti/TestCommon.hs | 4 ++-- > 7 files changed, 30 insertions(+), 21 deletions(-) > > diff --git a/lib/cli.py b/lib/cli.py > index 462afa3..d86c5c0 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -464,7 +464,7 @@ def _ExtractTagsObject(opts, args): > raise errors.ProgrammerError("tag_type not passed to > _ExtractTagsObject") > kind = opts.tag_type > if kind == constants.TAG_CLUSTER: > - retval = kind, None > + retval = kind, "" > elif kind in (constants.TAG_NODEGROUP, > constants.TAG_NODE, > constants.TAG_NETWORK, > diff --git a/lib/cmdlib/instance_operation.py > b/lib/cmdlib/instance_operation.py > index 77699fb..a9fcc7b 100644 > --- a/lib/cmdlib/instance_operation.py > +++ b/lib/cmdlib/instance_operation.py > @@ -184,7 +184,7 @@ class LUInstanceShutdown(LogicalUnit): > > """ > env = BuildInstanceHookEnvByObject(self, self.instance) > - env["SHUTDOWN_TIMEOUT"] = self.op.shutdown_timeout > + env["TIMEOUT"] = self.op.timeout > If I understood correctly, you are reverting a merge/rebase problem you introduced earlier on, right? At the very least, please mention it in the commit description, so people are not puzzled when they git blame or something. > return env > > def BuildHooksNodes(self): > @@ -233,7 +233,7 @@ class LUInstanceShutdown(LogicalUnit): > result = self.rpc.call_instance_shutdown( > self.instance.primary_node, > self.instance, > - self.op.shutdown_timeout, self.op.reason) > + self.op.timeout, self.op.reason) > See above, please mention in commit message. > msg = result.fail_msg > if msg: > self.LogWarning("Could not shutdown instance: %s", msg) > diff --git a/src/Ganeti/HTools/Program/Harep.hs > b/src/Ganeti/HTools/Program/Harep.hs > index 386aa8c..94fb781 100644 > --- a/src/Ganeti/HTools/Program/Harep.hs > +++ b/src/Ganeti/HTools/Program/Harep.hs > @@ -268,14 +268,12 @@ commitChange client instData = do > when (isJust arData) $ do > let tag = arTag $ fromJust arData > putStrLn (">>> Adding the following tag to " ++ iname ++ ":\n" ++ > show tag) > - tagName <- mkNonEmpty iname > - execJobsWaitOk' [OpTagsSet TagKindInstance [tag] (Just tagName)] > + execJobsWaitOk' [OpTagsSet TagKindInstance [tag] (Just iname)] > > unless (null rmTags) $ do > putStr (">>> Removing the following tags from " ++ iname ++ ":\n" ++ > unlines (map show rmTags)) > - tagName <- mkNonEmpty iname > - execJobsWaitOk' [OpTagsDel TagKindInstance rmTags (Just tagName)] > + execJobsWaitOk' [OpTagsDel TagKindInstance rmTags (Just iname)] > > return instData { tagsToRemove = [] } > > diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs > index 764cce4..45dec90 100644 > --- a/src/Ganeti/OpCodes.hs > +++ b/src/Ganeti/OpCodes.hs > @@ -530,7 +530,7 @@ $(genOpCode "OpCode" > , pInstanceUuid > , pForce > , pIgnoreOfflineNodes > - , pShutdownTimeout > + , pShutdownTimeout' > , pNoRemember > ], > "instance_name") > @@ -856,7 +856,7 @@ $(genOpCode "OpCode" > ], > "duration") > , ("OpTestAllocator", > - [t| () |], > + [t| String |], > OpDoc.opTestAllocator, > [ pIAllocatorDirection > , pIAllocatorMode > @@ -878,7 +878,7 @@ $(genOpCode "OpCode" > ], > "iallocator") > , ("OpTestJqueue", > - [t| () |], > + [t| Bool |], > OpDoc.opTestJqueue, > [ pJQueueNotifyWaitLock > , pJQueueNotifyExec > diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs > index 8f54b3f..80b23bc 100644 > --- a/src/Ganeti/OpParams.hs > +++ b/src/Ganeti/OpParams.hs > @@ -52,6 +52,7 @@ module Ganeti.OpParams > , pTagsName > , pOutputFields > , pShutdownTimeout > + , pShutdownTimeout' > , pShutdownInstance > , pForce > , pIgnoreOfflineNodes > @@ -572,7 +573,7 @@ pEnabledHypervisors :: Field > pEnabledHypervisors = > withDoc "List of enabled hypervisors" . > optionalField $ > - simpleField "enabled_hypervisors" [t| Hypervisor |] > + simpleField "enabled_hypervisors" [t| [Hypervisor] |] > > pClusterHvParams :: Field > pClusterHvParams = > @@ -616,19 +617,19 @@ pUidPool :: Field > pUidPool = > withDoc "Set UID pool, must be list of lists describing UID ranges\ > \ (two items, start and end inclusive)" . > - optionalField $ simpleField "uid_pool" [t| [[(Int, Int)]] |] > + optionalField $ simpleField "uid_pool" [t| [(Int, Int)] |] > > pAddUids :: Field > pAddUids = > withDoc "Extend UID pool, must be list of lists describing UID\ > \ ranges (two items, start and end inclusive)" . > - optionalField $ simpleField "add_uids" [t| [[(Int, Int)]] |] > + optionalField $ simpleField "add_uids" [t| [(Int, Int)] |] > > pRemoveUids :: Field > pRemoveUids = > withDoc "Shrink UID pool, must be list of lists describing UID\ > \ ranges (two items, start and end inclusive) to be removed" . > - optionalField $ simpleField "remove_uids" [t| [[(Int, Int)]] |] > + optionalField $ simpleField "remove_uids" [t| [(Int, Int)] |] > > pMaintainNodeHealth :: Field > pMaintainNodeHealth = > @@ -704,7 +705,7 @@ pEnabledDiskTemplates :: Field > pEnabledDiskTemplates = > withDoc "List of enabled disk templates" . > optionalField $ > - simpleField "enabled_disk_templates" [t| DiskTemplate |] > + simpleField "enabled_disk_templates" [t| [DiskTemplate] |] > > pQueryWhat :: Field > pQueryWhat = > @@ -796,7 +797,8 @@ pNodeUuid = > > pPrimaryIp :: Field > pPrimaryIp = > - withDoc "Primary IP address" $ > + withDoc "Primary IP address" . > + optionalField $ > simpleField "primary_ip" [t| NonEmptyString |] > > pSecondaryIp :: Field > @@ -979,7 +981,7 @@ pInstDisks = > > pDiskTemplate :: Field > pDiskTemplate = > - withDoc "List of instance disks" $ > + withDoc "Disk template" $ > simpleField "disk_template" [t| DiskTemplate |] > > pFileDriver :: Field > @@ -1151,6 +1153,15 @@ pShutdownTimeout = > defaultField [| forceNonNeg C.defaultShutdownTimeout |] $ > simpleField "shutdown_timeout" [t| NonNegative Int |] > > +-- | Another name for the shutdown timeout, because we like to be > +-- inconsistent. > Could we just change the name to something sensibly? How does it differ from pShutdownTimeout? As far as I have seen the only difference is, that the actual field is renamed and is called "timeout" instead of "shutdown_timeout". So why not go for pTimeout? Or pInstShutdownTimeout? Or even pShutdownTimeoutRenamedToInstShutdownTimeout? I don't really care, every _descriptive_ name would be better than an ironic comment ;-). > +pShutdownTimeout' :: Field > +pShutdownTimeout' = > + withDoc "How long to wait for instance to shut down" . > + renameField "InstShutdownTimeout" . > + defaultField [| forceNonNeg C.defaultShutdownTimeout |] $ > + simpleField "timeout" [t| NonNegative Int |] > + > pIgnoreFailures :: Field > pIgnoreFailures = > withDoc "Whether to ignore failures during removal" $ > @@ -1373,7 +1384,7 @@ pTagsName :: Field > pTagsName = > withDoc "Name of object" . > renameField "TagsGetName" . > - optionalField $ simpleField "name" [t| NonEmptyString |] > + optionalField $ simpleField "name" [t| String |] > > pTagsList :: Field > pTagsList = > diff --git a/test/hs/Test/Ganeti/OpCodes.hs > b/test/hs/Test/Ganeti/OpCodes.hs > index d8f3f83..75162cb 100644 > --- a/test/hs/Test/Ganeti/OpCodes.hs > +++ b/test/hs/Test/Ganeti/OpCodes.hs > @@ -197,7 +197,7 @@ instance Arbitrary OpCodes.OpCode where > OpCodes.OpNodeRemove <$> genNodeNameNE <*> return Nothing > "OP_NODE_ADD" -> > OpCodes.OpNodeAdd <$> genNodeNameNE <*> emptyMUD <*> emptyMUD <*> > - genNameNE <*> genMaybe genNameNE <*> arbitrary <*> > + genMaybe genNameNE <*> genMaybe genNameNE <*> arbitrary <*> > genMaybe genNameNE <*> arbitrary <*> arbitrary <*> emptyMUD > "OP_NODE_QUERY" -> > OpCodes.OpNodeQuery <$> genFieldsNE <*> genNamesNE <*> arbitrary > diff --git a/test/hs/Test/Ganeti/TestCommon.hs > b/test/hs/Test/Ganeti/TestCommon.hs > index ce266c9..2edbdad 100644 > --- a/test/hs/Test/Ganeti/TestCommon.hs > +++ b/test/hs/Test/Ganeti/TestCommon.hs > @@ -328,9 +328,9 @@ genIp6Net = do > > -- | Generates a valid, arbitrary tag name with respect to the given > -- 'TagKind' for opcodes. > -genOpCodesTagName :: TagKind -> Gen (Maybe NonEmptyString) > +genOpCodesTagName :: TagKind -> Gen (Maybe String) > genOpCodesTagName TagKindCluster = return Nothing > -genOpCodesTagName _ = Just <$> (mkNonEmpty =<< genFQDN) > +genOpCodesTagName _ = Just <$> genFQDN > > -- | Generates a valid, arbitrary tag name with respect to the given > -- 'TagKind' for Luxi. > -- > 1.8.3 > > Rest LGTM, thanks, Thomas. -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
