From 5cfb20668421acb3c0de133afe973fc693b8b320 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sat, 5 Dec 2015 18:03:02 -0400 Subject: remove trivial --- debian/changelog | 5 +++++ .../comment_1_c02e8783b91c3c0326bf1b317be4694f._comment | 11 +++++------ doc/writing_properties.mdwn | 9 ++++----- src/Propellor/Property.hs | 14 -------------- src/Propellor/Property/Apt.hs | 8 ++++---- src/Propellor/Property/Nginx.hs | 2 +- src/Propellor/Property/Postfix.hs | 3 ++- src/Propellor/Property/Prosody.hs | 2 +- src/Propellor/Property/Uwsgi.hs | 2 +- src/Propellor/Types/ResultCheck.hs | 4 ++-- 10 files changed, 25 insertions(+), 35 deletions(-) diff --git a/debian/changelog b/debian/changelog index 1da30f22..e09cfbac 100644 --- a/debian/changelog +++ b/debian/changelog @@ -14,6 +14,11 @@ propellor (2.15.0) UNRELEASED; urgency=medium change is always safe to make. - Or, if you know that the command should modifiy a file, use: `changesFile` filename + * The `trivial` combinator has been removed. (API change) + Instead, use: + `assume` NoChange + Or, better, use changesFile or checkResult to accurately report + when a property makes a change. * A few properties have had their Result improved, for example Apt.buldDep and Apt.autoRemove now check if a change was made or not. * User.hasDesktopGroups changed to avoid trying to add the user to diff --git a/doc/todo/Wishlist:_User.hasLoginShell/comment_1_c02e8783b91c3c0326bf1b317be4694f._comment b/doc/todo/Wishlist:_User.hasLoginShell/comment_1_c02e8783b91c3c0326bf1b317be4694f._comment index 52043406..91ca404e 100644 --- a/doc/todo/Wishlist:_User.hasLoginShell/comment_1_c02e8783b91c3c0326bf1b317be4694f._comment +++ b/doc/todo/Wishlist:_User.hasLoginShell/comment_1_c02e8783b91c3c0326bf1b317be4694f._comment @@ -25,17 +25,16 @@ run: shellSetTo :: UserName -> FilePath -> Property shellSetTo user shell = cmdProperty "chsh" ["--shell", shell, user] - +: The only remaining problem with this is that shellSetTo runs chsh every time, and propellor will always display that it's made a change each time it runs, even when it didn't really do much. Now, there's an easy way to -avoid that problem, we could just tell propellor that it's a trivial -property, and then it will run chsh every time and not think it made any -change: +avoid that problem, we could just tell propellor to assume it's not made +any change: shellSetTo :: UserName -> FilePath -> Property - shellSetTo user shell = trivial $ - cmdProperty "chsh" ["--shell", shell, user] + shellSetTo user shell = cmdProperty "chsh" ["--shell", shell, user] + `assume` NoChange But, it's not much harder to do this right. Let's make the property check if the user's shell is already set to the desired value and avoid diff --git a/doc/writing_properties.mdwn b/doc/writing_properties.mdwn index 9a700ac3..2209026f 100644 --- a/doc/writing_properties.mdwn +++ b/doc/writing_properties.mdwn @@ -53,13 +53,12 @@ run: The only remaining problem with this is that shellSetTo runs chsh every time, and propellor will always display that it's made a change each time it runs, even when it didn't really do much. Now, there's an easy way to -avoid that problem, we could just tell propellor that it's a trivial -property, and then it will run chsh every time and not think it made any -change: +avoid that problem, we could just tell propellor to assume that chsh +has not made a change: shellSetTo :: UserName -> FilePath -> Property - shellSetTo user shell = trivial $ - cmdProperty "chsh" ["--shell", shell, user] + shellSetTo user shell = cmdProperty "chsh" ["--shell", shell, user] + `assume` NoChange But, it's not much harder to do this right. Let's make the property check if the user's shell is already set to the desired value and avoid diff --git a/src/Propellor/Property.hs b/src/Propellor/Property.hs index c57ef2f0..2976acf1 100644 --- a/src/Propellor/Property.hs +++ b/src/Propellor/Property.hs @@ -11,7 +11,6 @@ module Propellor.Property ( , flagFile' , check , fallback - , trivial , revert -- * Property descriptions , describe @@ -185,19 +184,6 @@ fallback = combineWith combiner revertcombiner else return r revertcombiner = (<>) --- | Marks a Property as trivial. It can only return FailedChange or --- NoChange. --- --- Useful when it's just as expensive to check if a change needs --- to be made as it is to just idempotently assure the property is --- satisfied. For example, chmodding a file. -trivial :: Property i -> Property i -trivial p = adjustPropertySatisfy p $ \satisfy -> do - r <- satisfy - if r == MadeChange - then return NoChange - else return r - -- | Indicates that a Property may change a particular file. When the file -- is modified, the property will return MadeChange instead of NoChange. changesFile :: Checkable p i => p i -> FilePath -> Property i diff --git a/src/Propellor/Property/Apt.hs b/src/Propellor/Property/Apt.hs index cf81c9be..26c151d9 100644 --- a/src/Propellor/Property/Apt.hs +++ b/src/Propellor/Property/Apt.hs @@ -140,13 +140,13 @@ installed' params ps = robustly $ check (isInstallable ps) go `assume` MadeChange installedBackport :: [Package] -> Property NoInfo -installedBackport ps = trivial $ withOS desc $ \o -> case o of +installedBackport ps = withOS desc $ \o -> case o of Nothing -> error "cannot install backports; os not declared" (Just (System (Debian suite) _)) -> case backportSuite suite of Nothing -> notsupported o - Just bs -> ensureProperty $ runApt - (["install", "-t", bs, "-y"] ++ ps) - `assume` MadeChange + Just bs -> ensureProperty $ + runApt (["install", "-t", bs, "-y"] ++ ps) + `changesFile` dpkgStatus _ -> notsupported o where desc = (unwords $ "apt installed backport":ps) diff --git a/src/Propellor/Property/Nginx.hs b/src/Propellor/Property/Nginx.hs index c28dcc01..8fb5c49b 100644 --- a/src/Propellor/Property/Nginx.hs +++ b/src/Propellor/Property/Nginx.hs @@ -17,7 +17,7 @@ siteEnabled hn cf = enable disable `requires` siteAvailable hn cf `requires` installed `onChange` reloaded - disable = trivial $ File.notPresent (siteVal hn) + disable = File.notPresent (siteVal hn) `describe` ("nginx site disable" ++ hn) `requires` installed `onChange` reloaded diff --git a/src/Propellor/Property/Postfix.hs b/src/Propellor/Property/Postfix.hs index 782e7f45..bc46ac21 100644 --- a/src/Propellor/Property/Postfix.hs +++ b/src/Propellor/Property/Postfix.hs @@ -32,7 +32,7 @@ satellite :: Property NoInfo satellite = check (not <$> mainCfIsSet "relayhost") setup `requires` installed where - setup = trivial $ property "postfix satellite system" $ do + setup = property "postfix satellite system" $ do hn <- asks hostName let (_, domain) = separate (== '.') hn ensureProperties @@ -181,3 +181,4 @@ saslPasswdSet domain (User user) = withPrivData src ctx $ \getpw -> trivial $ p = proc "saslpasswd2" ps ctx = Context "sasl" src = PrivDataSource (Password uatd) "enter password" + trivial = flip assume NoChange diff --git a/src/Propellor/Property/Prosody.hs b/src/Propellor/Property/Prosody.hs index f2d80ae4..47095504 100644 --- a/src/Propellor/Property/Prosody.hs +++ b/src/Propellor/Property/Prosody.hs @@ -24,7 +24,7 @@ confEnabled conf cf = enable disable dir = confValPath conf confValRelativePath conf' = File.LinkTarget $ "../conf.avail" conf' <.> "cfg.lua" - disable = trivial $ File.notPresent (confValPath conf) + disable = File.notPresent (confValPath conf) `describe` ("prosody conf disabled " ++ conf) `requires` installed `onChange` reloaded diff --git a/src/Propellor/Property/Uwsgi.hs b/src/Propellor/Property/Uwsgi.hs index 9748f16d..8b531c3f 100644 --- a/src/Propellor/Property/Uwsgi.hs +++ b/src/Propellor/Property/Uwsgi.hs @@ -19,7 +19,7 @@ appEnabled an cf = enable disable `requires` appAvailable an cf `requires` installed `onChange` reloaded - disable = trivial $ File.notPresent (appVal an) + disable = File.notPresent (appVal an) `describe` ("uwsgi app disable" ++ an) `requires` installed `onChange` reloaded diff --git a/src/Propellor/Types/ResultCheck.hs b/src/Propellor/Types/ResultCheck.hs index 590d4ab9..0c7597a2 100644 --- a/src/Propellor/Types/ResultCheck.hs +++ b/src/Propellor/Types/ResultCheck.hs @@ -60,7 +60,7 @@ instance Checkable UncheckedProperty i where -- -- However, beware assuming `NoChange`, as that will make combinators -- like `onChange` not work. -assume :: UncheckedProperty i -> Result -> Property i -assume (UncheckedProperty p) result = adjustPropertySatisfy (checkedProp p) $ \satisfy -> do +assume :: Checkable p i => p i -> Result -> Property i +assume p result = adjustPropertySatisfy (checkedProp p) $ \satisfy -> do r <- satisfy return (r <> result) -- cgit v1.2.3