On Tue, Feb 04, 2014 at 05:00:24PM +0100, Santi Raffa wrote:
> Signed-off-by: Santi Raffa <[email protected]>
> ---
> lib/cli.py | 12 ++++
> lib/config.py | 5 +-
> lib/ht.py | 19 +++++-
> lib/serializer.py | 138
> +++++++++++++++++++++++++++++++++++++++++
> src/Ganeti/JSON.hs | 1 +
> src/Ganeti/Types.hs | 47 ++++++++++++++
> test/hs/Test/Ganeti/Objects.hs | 3 +
> test/hs/Test/Ganeti/OpCodes.hs | 8 +++
> 8 files changed, 230 insertions(+), 3 deletions(-)
>
> diff --git a/lib/cli.py b/lib/cli.py
> index ad6c5bc..ce43a05 100644
> --- a/lib/cli.py
> +++ b/lib/cli.py
> @@ -44,6 +44,7 @@ from ganeti import netutils
> from ganeti import qlang
> from ganeti import objects
> from ganeti import pathutils
> +from ganeti import serializer
>
> from ganeti.runtime import (GetClient)
>
> @@ -678,6 +679,15 @@ def check_key_val(option, opt, value): # pylint:
> disable=W0613
> return _SplitKeyVal(opt, value, True)
>
>
> +def check_key_private_val(option, opt, value): # pylint: disable=W0613
> + """Custom parser class for private and secret key=val,key=val options.
> +
> + This will store the parsed values as a dict {key: val}.
> +
> + """
> + return serializer.PrivateDict(_SplitKeyVal(opt, value, True))
> +
> +
> def _SplitListKeyVal(opt, value):
> retval = {}
> for elem in value.split("/"):
> @@ -780,6 +790,7 @@ class CliOption(Option):
> "multilistidentkeyval",
> "identkeyval",
> "keyval",
> + "keyprivateval",
> "unit",
> "bool",
> "list",
> @@ -789,6 +800,7 @@ class CliOption(Option):
> TYPE_CHECKER["multilistidentkeyval"] = check_multilist_ident_key_val
> TYPE_CHECKER["identkeyval"] = check_ident_key_val
> TYPE_CHECKER["keyval"] = check_key_val
> + TYPE_CHECKER["keyprivateval"] = check_key_private_val
> TYPE_CHECKER["unit"] = check_unit
> TYPE_CHECKER["bool"] = check_bool
> TYPE_CHECKER["list"] = check_list
> diff --git a/lib/config.py b/lib/config.py
> index fcdefc3..67a117c 100644
> --- a/lib/config.py
> +++ b/lib/config.py
> @@ -2488,7 +2488,10 @@ class ConfigWriter(object):
> if destination is None:
> destination = self._cfg_file
> self._BumpSerialNo()
> - txt = serializer.Dump(self._config_data.ToDict())
> + txt = serializer.DumpJson(
> + self._config_data.ToDict(_with_private=True),
> + private_decoder=serializer.EncodeWithPrivateFields
> + )
>
> getents = self._getents()
> try:
> diff --git a/lib/ht.py b/lib/ht.py
> index 500c391..5f271f9 100644
> --- a/lib/ht.py
> +++ b/lib/ht.py
> @@ -29,7 +29,7 @@ from ganeti import compat
> from ganeti import utils
> from ganeti import constants
> from ganeti import objects
> -
> +from ganeti.serializer import Private, PrivateDict
>
> _PAREN_RE = re.compile("^[a-zA-Z0-9_-]+$")
>
> @@ -77,6 +77,9 @@ class _DescWrapper(_WrapperBase):
> def __str__(self):
> return self._text
>
> + def __repr__(self):
> + return "<%s %r>" % (self._text, self._fn)
> +
>
> class _CommentWrapper(_WrapperBase):
> """Wrapper class for comment.
> @@ -270,7 +273,7 @@ def TDict(val):
> """Checks if the given value is a dictionary.
>
> """
> - return isinstance(val, dict)
> + return isinstance(val, dict) or isinstance(val, PrivateDict)
Given that it is somewhat strange to have an object posing a
dictionary, I suggest adding an explanation to the docstring.
> def TIsLength(size):
> @@ -416,6 +419,18 @@ def TInstanceOf(cls):
> return desc(lambda val: isinstance(val, cls))
>
>
> +def TPrivate(val_type):
> + """Checks if a given value is an instance of Private.
> +
> + """
> + def fn(val):
> + return isinstance(val, Private) and val_type(val.Get())
> +
> + desc = WithDesc("Private %s" % Parens(val_type))
> +
> + return desc(fn)
> +
> +
> def TListOf(my_type):
> """Checks if a given value is a list with all elements of the same type.
>
> diff --git a/lib/serializer.py b/lib/serializer.py
> index f008155..7bd4c1f 100644
> --- a/lib/serializer.py
> +++ b/lib/serializer.py
> @@ -166,3 +166,141 @@ Dump = DumpJson
> Load = LoadJson
> DumpSigned = DumpSignedJson
> LoadSigned = LoadSignedJson
> +
> +
> +class Private(object):
> +
Missing docstring.
> + def __init__(self, item, descr="redacted"):
> + if isinstance(item, Private):
> + raise ValueError("Attempted to nest Private values.")
> + self._item = item
> + self._descr = descr
How about the following:
We can unwrap the item from the inner private and use that as this
object's item. That way we can eliminate the possibility of the
runtime error. By the way, a recursive function here, which
traverses the Private.item references, can make this more robust.
> +
> + def Get(self):
> + return self._item
> +
> + def __str__(self):
> + return "<{._descr}>".format(self)
> +
> + def __repr__(self):
> + return "Private(?, descr='{._descr}')".format(self)
> +
> + # pylint: disable=W0212
> + # If it doesn't access _item directly, the call will go through __getattr__
> + # because this class defines __slots__ and "item" is not in it.
> + # OTOH, if we do add it there, we'd risk shadowing an "item" attribute.
> + def __eq__(self, other):
> + if isinstance(other, Private):
> + return self._item == other._item
> + else:
> + return self._item == other
> +
> + def __hash__(self):
> + return hash(self._item)
> +
> + def __format__(self, *_1, **_2):
> + return self.__str__()
> +
> + def __getattr__(self, attr):
> + return Private(getattr(self._item, attr),
> + descr="%s.%s" % (self._descr, attr))
> +
> + def __call__(self, *args, **kwargs):
> + return Private(self._item(*args, **kwargs),
> + descr="%s()" % self._descr)
> +
> + # pylint: disable=R0201
> + # While this could get away with being a function, it needs to be an attr.
Perhaps rename attr to method.
> + # Required by the copy.deepcopy function used by FillDict.
> + def __getnewargs__(self):
> + return tuple()
> +
> + def __nonzero__(self):
> + return bool(self._item)
> +
> + # Get in the way of Pickle by implementing __slots__ but not __getstate__
> + # ...and get a performance boost, too.
> + __slots__ = ["_item", "_descr"]
> +
> +
> +class PrivateDict(dict):
> + """A dictionary that turns its values to private fields.
> +
Missing "Example::" and indentation (I think)
> + >>> PrivateDict()
> + {}
> + >>> supersekkrit = PrivateDict({"password": "foobar"})
> + >>> supersekkrit["password"]
> + Private(?, descr='password')
> + >>> supersekkrit["password"].Get()
> + 'foobar'
> + >>> supersekkrit["user"] = "eggspam"
> + >>> supersekkrit.GetPrivate("user")
> + 'eggspam'
> + >>> supersekkrit.Unprivate()
> + {'password': 'foobar', 'user': 'eggspam'}
Missing newline before the end of the docstring.
> + """
> +
Extra newline after the end of the docstring.
> + def __init__(self, data=None):
> + dict.__init__(self)
> + self.update(data)
> +
> + def __setitem__(self, item, value):
> + if not isinstance(value, Private):
> + if not isinstance(item, dict):
> + value = Private(value, descr=item)
> + else:
> + value = PrivateDict(value)
> + dict.__setitem__(self, item, value)
> +
> + # copied straight from cpython/Lib/UserDict.py
> + # The actual conversion to Private containers is done by __setitem__
> + def update(self, other=None, **kwargs):
> + # Make progressively weaker assumptions about "other"
> + if other is None:
> + pass
> + elif hasattr(other, 'iteritems'): # iteritems saves memory and lookups
> + for k, v in other.iteritems():
> + self[k] = v
> + elif hasattr(other, 'keys'):
> + for k in other.keys():
> + self[k] = other[k]
> + else:
> + for k, v in other:
> + self[k] = v
> + if kwargs:
> + self.update(kwargs)
> +
> + def GetPrivate(self, *args):
> + """Like dict.get, but extracting the value in the process.
> +
> + Arguments are semantically equivalent to dict.get
> +
Needs "Example::"
> + >>> PrivateDict({"foo": "bar"}).GetPrivate("foo")
> + 'bar'
> + >>> PrivateDict({"foo": "bar"}).GetPrivate("baz", "spam")
> + 'spam'
Missing newline before the end of docstring.
> + """ # epydoc does not check doctests, btw.
This is interesting... Might this be an issue?
In any case, don't put it here.
> + if len(args) == 1:
> + key, = args
> + return self[key].Get()
> + elif len(args) == 2:
> + key, default = args
> + if key not in self:
> + return default
> + else:
> + return self[key].Get()
> + else:
> + raise TypeError("GetPrivate() takes 2 arguments (%d given)" %
> len(args))
> +
> + def Unprivate(self):
> + """Turn this dict of Private() values to a dict of values.
> +
Needs "Example::"
> + >>> PrivateDict({"foo": "bar"}).Unprivate()
> + {'foo': 'bar'}
> +
> + @rtype: dict
Needs newline.
> + """
> + returndict = {}
> + for key in self:
> + returndict[key] = self[key].Get()
> + return returndict
> diff --git a/src/Ganeti/JSON.hs b/src/Ganeti/JSON.hs
> index c1daa5a..fe84d8a 100644
> --- a/src/Ganeti/JSON.hs
> +++ b/src/Ganeti/JSON.hs
> @@ -48,6 +48,7 @@ module Ganeti.JSON
> , toArray
> , optionalJSField
> , optFieldsToObj
> + , readContainer
> , HasStringRepr(..)
> , GenericContainer(..)
> , Container
> diff --git a/src/Ganeti/Types.hs b/src/Ganeti/Types.hs
> index 2050cf1..28961dd 100644
> --- a/src/Ganeti/Types.hs
> +++ b/src/Ganeti/Types.hs
> @@ -161,8 +161,11 @@ module Ganeti.Types
> , hotplugActionToRaw
> , ParamVisibility(..)
> , paramVisibilityToRaw
> + , Private(..)
> + , showPrivateJSObject
> ) where
>
> +import Control.Applicative
> import Control.Monad (liftM)
> import qualified Text.JSON as JSON
> import Text.JSON (JSON, readJSON, showJSON)
> @@ -903,3 +906,47 @@ $(THH.makeJSONInstance ''ParamVisibility)
>
> instance THH.PyValue ParamVisibility where
> showValue = show . paramVisibilityToRaw
It would be nice to add a Haddock section here, like so
-- * Private type and instances
> +
> +-- | A container for values that should be happy to be manipulated yet
> +-- | refuses to be shown unless explicitly requested.
> +
The format is as follows (pay attention to the bars and the newlines):
-- | Doc ...
-- no bar at this level ...
newtype Private ...
> +newtype Private value = Private { getPrivate :: value }
> + deriving Eq
s/value/a/
> +
> +instance (Show a, JSON.JSON a) => JSON.JSON (Private a) where
> + readJSON json = Private <$> JSON.readJSON json
If you could write this line in point-free style, it would be really good.
> + showJSON (Private a) = JSON.showJSON a
Instead
(Private x) = JSON.showJSON x
> +
> +-- It would be better not to implement this at all.
> +-- Alas, Show OpCode requires Show Private.
Format problem.
> +instance (Show a) => Show (Private a) where
No parenthesis here (Show a)
instance Show a => Show (Private a) where
> + show _ = "<redacted>"
> +
> +instance (THH.PyValue a) => THH.PyValue (Private a) where
No parens here as well (on the left side)
> + showValue (Private a) = "Private(" ++ THH.showValue a ++ ")"
> +
> +instance Functor Private where
> + fmap f (Private x) = Private $ f x
> +
> +instance Monad Private where
> + (Private x) >>= f = f x
> + return = Private
> +
> +--instance (JSON.JSON a) => PrivateJSON (Private a) where
> +-- unprivateAndShowJSON l | l >= Private_ = JSON.showJSON . getPrivate
> +-- unprivateAndShowJSON _ = JSON.showJSON
> +
> +--instance (JSON.JSON a) => JSON.JSON [(String, Private JSON.JSValue)] where
> +-- showJSON value = JSON.toJSObject $ map f value
> +-- where f (k, v) = (k, Private $ JSON.showJSON v)
> +
> +showPrivateJSObject :: (JSON.JSON a) =>
> + [(String, a)] -> JSON.JSObject (Private JSON.JSValue)
> +showPrivateJSObject value = JSON.toJSObject $ map f value
> + where f (k, v) = (k, Private $ JSON.showJSON v)
> +
> +--readPrivateJSObject :: String ->
> +-- JSON.Result (JSON.JSObject (Private JSON.JSValue))
> +--readPrivateJSObject json = do
> +-- jsobj <- JSON.decode json
> +-- return fmap Private $ readContainer jsobj
> diff --git a/test/hs/Test/Ganeti/Objects.hs b/test/hs/Test/Ganeti/Objects.hs
> index 50a5efb..dbae17f 100644
> --- a/test/hs/Test/Ganeti/Objects.hs
> +++ b/test/hs/Test/Ganeti/Objects.hs
> @@ -219,6 +219,9 @@ instance Arbitrary ClusterNicParams where
> instance Arbitrary OsParams where
> arbitrary = (GenericContainer . Map.fromList) <$> arbitrary
>
> +instance (Arbitrary a) => Arbitrary (Private a) where
No parens on the left side.
> + arbitrary = Private <$> arbitrary
> +
> instance Arbitrary ClusterOsParams where
> arbitrary = (GenericContainer . Map.fromList) <$> arbitrary
>
> diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.hs
> index e0a3b69..0719ae1 100644
> --- a/test/hs/Test/Ganeti/OpCodes.hs
> +++ b/test/hs/Test/Ganeti/OpCodes.hs
> @@ -430,6 +430,14 @@ genMacPrefix = do
> octets <- vectorOf 3 $ choose (0::Int, 255)
> mkNonEmpty . intercalate ":" $ map (printf "%02x") octets
>
> +-- | JSObject of arbitrary data.
Newline here between the short description and the big description.
Thanks,
Jose
> +-- Since JSValue does not implement Arbitrary, I'll simply generate
> +-- (String, String) objects.
> +arbitraryPrivateJSObj :: Gen (J.JSObject (Private J.JSValue))
> +arbitraryPrivateJSObj = constructor <$> (fromNonEmpty <$> genNameNE)
> + <*> (fromNonEmpty <$> genNameNE)
> + where constructor k v = showPrivateJSObject [(k, v)]
> +
body of the function should go below the definition, and not to the
right of equals (=). For example,
arbitraryPrivateJSObj =
body goes here
Also, you can make constructor point-free, for example (untested)
where constructor = showPrivateJSObject . [] . (,)
Thanks,
Jose
> -- | Arbitrary instance for MetaOpCode, defined here due to TH ordering.
> $(genArbitrary ''OpCodes.MetaOpCode)
>
> --
> 1.9.0.rc1.175.g0b1dcb5
>
--
Jose Antonio Lopes
Ganeti Engineering
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
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
