add KVStoreClient, examples #28
Conversation
.pre-commit-config.yaml
Outdated
| @@ -27,6 +27,6 @@ repos: | |||
| name: mypy | |||
| stages: [commit] | |||
| language: system | |||
| entry: poetry run mypy **/*.py | |||
| entry: poetry run mypy pykeybasebot/ | |||
There was a problem hiding this comment.
I was getting "No directory found" for **/*.py. Also I think this should match with what is in the Makefile for make test
There was a problem hiding this comment.
we could do poetry run mypy pykeybasebot examples tests (though i think i was getting errors with the tests dir)
examples/6_totp.py
Outdated
|
|
||
| logging.basicConfig(level=logging.DEBUG) | ||
|
|
||
| if "win32" in sys.platform: |
There was a problem hiding this comment.
ugh. we need to figure out how to pull this down into the framework. not for this PR. just saying it so it's not only in my head. these examples should have as little boilerplate as possible.
examples/5_secret_storage.py
Outdated
| cached = self.cache[entry_key].copy() if entry_key in self.cache else None | ||
| if cached is not None: | ||
| cached["info"] = ( | ||
| cached["info"].copy() if cached["info"] is not None else None |
There was a problem hiding this comment.
can we do a deepcopy of the self.cache[entry_key] instead?
| #!/usr/bin/env python3 | ||
|
|
||
| ################################### | ||
| # WHAT IS IN THIS EXAMPLE? |
There was a problem hiding this comment.
i really like the way this example came out :)
examples/5_secret_storage.py
Outdated
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| self.secret_kvstore_client = SecretKeyKVStoreClient(self) |
There was a problem hiding this comment.
this is never referenced from the CachedBot (which is good, because it would be an implicit violation of the law of demeter). i'd suggest make this constraint explicit...
def __init__(self, *args, **kwargs):
secret_client = SecretKeyKVStoreClient(self)
self._cached_secret_kvstore = CachedKVStoreClient(self, secret_client)
super().__init__(*args, **kwargs)
@property
def cached_secret_kvstore(self):
return self._cached_secret_kvstoreif you're feeling bold, you could also make it like this...
@property
def kvstore(self):
return self._cached_secret_kvstorewhich might simplify some of the other objects higher up in the stack: i.e. your RentalClient just needs the bot and it can interact with it like it would a normal bot, this one has just been juiced up.
given the way this bot object has developed, i'd also consider renaming it from CachedBot to something a little more telling. like maybe CustomBot or JuicedUpBot or PrivateConcurrentBot? dunno. CachedBot is a bit misleading.
There was a problem hiding this comment.
after later comments, adding another suggestion...
class WhateverYouChooseBot(Bot):
@property
def kvstore(self):
if not self._cached_secret_kvstore
basic_client = KVStoreClient(self)
secret_client = SecretKeyKVStoreClient(basic_client)
self._cached_secret_kvstore = CachedKVStoreClient(secret_client)
return self._cached_secret_kvstoreThere was a problem hiding this comment.
ok i renamed the bot class to CustomKVStoreBot, and this bot has a kvstore property that returns a CachedKVStoreClient.
then i renamed RentalClient to RentalBotClient. and i made RentalBotClient take in a Bot.
examples/5_secret_storage.py
Outdated
| except RevisionError: | ||
| # refresh cached value | ||
| curr_info = await self.get(team, namespace, entry_key) | ||
| return curr_info # failed put. return KVGetResult. |
There was a problem hiding this comment.
not sure i love this pattern. a PUT could return the PUT result OR a GET result?
not a blocker. i just think this is a little bit of a weird API.
| self.secrets[team][namespace] = secret | ||
| return self.secrets[team][namespace] | ||
|
|
||
| async def hmac_key(self, team, namespace, entry_key) -> str: |
There was a problem hiding this comment.
NAB: it's probably a better practice to call this method something like hidden_key. it's only incidental that it happens to do an hmac to achieve those ends.
| return b64decode(x.encode("utf-8")) | ||
|
|
||
|
|
||
| class SecretKeyKVStoreClient: |
There was a problem hiding this comment.
i really like the encapsulation of responsibility that this object now has. 👍
examples/5_secret_storage.py
Outdated
|
|
||
| NAMESPACE = "rental" | ||
|
|
||
| def __init__(self, kvstore: CachedKVStoreClient): |
There was a problem hiding this comment.
i don't know much about duck typing with python's type system, but this would be a case for it. it looks to me like this could take a CachedKVStoreClient or a SecretClient or a regular ol' client.
oh actually, what i suggested before was to pass in a Bot to this thing. yeah. in which case the ducktyping would be any object that has a kvstore property.
this isn't important.
There was a problem hiding this comment.
ahh. i see this is calling additional methods on the kvstore that don't exist on the basic one. ok. nevermind.
There was a problem hiding this comment.
ok - i think i'd argue there's not really a compelling reason to expose get_cache to the outside world. the only reason something outside of the CachedKVStoreClient would want to call it is for performance. and that's not really the point of this thing, it's for concurrency. plus it adds branching logic in this client that's just not really necessary. so yeah. i think i'd argue:
- the cached kvstore client shouldn't expose
get_cachedto the outside world - the rental client should always just call
get(and now it never needs to worry about getting outdated data) - the rental client can be instantiated with anything kvstore-like. even if that's not a duck-typing exercise worth doing
examples/5_secret_storage.py
Outdated
| expected_revision = 1 | ||
| cached = self.kvstore.get_cached(tool) | ||
| if cached is not None: | ||
| if cached["info"]: |
There was a problem hiding this comment.
yeah - it's sub-optimal that the RentalClient knows about how the CachedClient arranges its internal data structures
|
|
||
|
|
||
| class CachedKVStoreClient: | ||
| class TryingKVStoreClient: |
examples/5_secret_storage.py
Outdated
| res = await self.lookup(team, tool) | ||
| info = ( | ||
| json.loads(res.entry_value) if res.entry_value != "" else {} | ||
| ) # if tool already exists, propagate existing info |
There was a problem hiding this comment.
interesting choice. no opinions. :)
examples/5_secret_storage.py
Outdated
| res = await self.kvstore.put( | ||
| team, self.NAMESPACE, tool, info, expected_revision | ||
| ) | ||
| return res |
There was a problem hiding this comment.
NAB: maybe here we should return nil because (1) you're good to go (and it's generally good practice to separate command from query methods), and (2) to distinguish it from an unsuccessful write
There was a problem hiding this comment.
or True/False for whether or not the reservation was accepted? (i.e. false if it's not accepted for an expected reason (i.e. someone else has that date))
There was a problem hiding this comment.
decided to change return type to Tuple[bool, Union[keybase1.KVGetResult, None]]
| if (day not in info) or (day in info and info[day] != user): | ||
| # failed to put because currently not reserved, or current reserver is not user | ||
| return res | ||
| expected_revision = res.revision + 1 | ||
| res = await self.kvstore.put( | ||
| team, self.NAMESPACE, tool, info, expected_revision | ||
| ) |
There was a problem hiding this comment.
github won't let me comment on the line below, but same thing
can't release until keybase/client is released with kv store feature
@AMarcedone take a look at 4_secret_storage.py; i implemented the solution we discussed