On Thu, Dec 13, 2012 at 06:12:25PM +0000, Dato Simó wrote:
> On Thu, Dec 13, 2012 at 13:27 +0100, Iustin Pop <[email protected]> wrote:
> > > +case_AutoRepairType_sort :: Assertion
> > > +case_AutoRepairType_sort = do
> > > +  let expected = [ Types.ArFixStorage
> > > +                 , Types.ArMigrate
> > > +                 , Types.ArFailover
> > > +                 , Types.ArReinstall
> > > +                 ]
> > > +      all_hs_raw = map Types.autoRepairTypeToRaw [minBound..maxBound]
> > > +  assertEqual "Haskell order" expected [minBound..maxBound]
> > > +  assertEqual "consistent with Python" C.autoRepairAllTypes all_hs_raw
> > > +
> > > +case_AutoRepairResult_pyequiv :: Assertion
> > > +case_AutoRepairResult_pyequiv = do
> > > +  let all_py_results = sort C.autoRepairAllResults
> > > +      all_hs_results = sort $
> > > +                       map Types.autoRepairResultToRaw 
> > > [minBound..maxBound]
> > > +  assertEqual "for AutoRepairResult equivalence" all_py_results 
> > > all_hs_results
> 
> > Mmm, please docstrings for these two properties (yes, they're very
> > straightforward, but…).
> 
> Ah, sure. Interdiff:
> 
> --- htest/Test/Ganeti/HTools/Types.hs
> +++ htest/Test/Ganeti/HTools/Types.hs
> @@ -149,6 +149,8 @@ prop_eitherToResult ei =
>                   Ok v' -> v == v'
>      where r = eitherToResult ei
>  
> +-- | Test 'AutoRepairType' ordering is as expected and consistent with Python
> +-- codebase.
>  case_AutoRepairType_sort :: Assertion
>  case_AutoRepairType_sort = do
>    let expected = [ Types.ArFixStorage
> @@ -160,6 +162,7 @@ case_AutoRepairType_sort = do
>    assertEqual "Haskell order" expected [minBound..maxBound]
>    assertEqual "consistent with Python" C.autoRepairAllTypes all_hs_raw
>  
> +-- | Test 'AutoRepairResult' type is equivalent with Python codebase.
>  case_AutoRepairResult_pyequiv :: Assertion
>  case_AutoRepairResult_pyequiv = do
>    let all_py_results = sort C.autoRepairAllResults
> 
> > > --- a/lib/constants.py
> > > +++ b/lib/constants.py
> > > @@ -2106,5 +2106,34 @@ NDS_NODE_DAEMON_CERTIFICATE = 
> > > "node_daemon_certificate"
> > >  NDS_SSCONF = "ssconf"
> > >  NDS_START_NODE_DAEMON = "start_node_daemon"
> 
> > > +# Auto-repair tag prefixes
> > > +AUTO_REPAIR_TAG_PREFIX = "ganeti:watcher:autorepair:"
> > > +AUTO_REPAIR_TAG_ENABLED = AUTO_REPAIR_TAG_PREFIX
> > > +AUTO_REPAIR_TAG_SUSPENDED = AUTO_REPAIR_TAG_ENABLED + "suspend:"
> > > +AUTO_REPAIR_TAG_PENDING = AUTO_REPAIR_TAG_PREFIX + "pending:"
> > > +AUTO_REPAIR_TAG_RESULT = AUTO_REPAIR_TAG_PREFIX + "result:"
> > > +
> > > +# Auto-repair levels
> > > +AUTO_REPAIR_FIX_STORAGE = "fix-storage"
> > > +AUTO_REPAIR_MIGRATE = "migrate"
> > > +AUTO_REPAIR_FAILOVER = "failover"
> > > +AUTO_REPAIR_REINSTALL = "reinstall"
> > > +AUTO_REPAIR_ALL_TYPES = [
> > > +  AUTO_REPAIR_FIX_STORAGE,
> > > +  AUTO_REPAIR_MIGRATE,
> > > +  AUTO_REPAIR_FAILOVER,
> > > +  AUTO_REPAIR_REINSTALL,
> > > +]
> > > +
> > > +# Auto-repair results
> > > +AUTO_REPAIR_SUCCESS = "success"
> > > +AUTO_REPAIR_FAILURE = "failure"
> > > +AUTO_REPAIR_ENOPERM = "enoperm"
> > > +AUTO_REPAIR_ALL_RESULTS = frozenset([
> > > +    AUTO_REPAIR_SUCCESS,
> > > +    AUTO_REPAIR_FAILURE,
> > > +    AUTO_REPAIR_ENOPERM,
> > > +])
> 
> > Actually, do these need to exist at all in the Python code? Will
> > anything look at them?
> 
> Right.
> 
> I don't think that nothing from Python is going to be using them, at
> least at first. But because these are string literals that have a public
> meaning (as opposed to internal data types), I thought it a good idea to
> continue using constants.py as the "source of truth", and have the work
> done if Python (say, cluster verify) ever needs to look at these.
> 
> I can revert if you would like, but I saw the integration as beneficial
> here.

Ack. Sounds good then, LGTM with the interdiff.

thanks,
iustin

Reply via email to