-
Notifications
You must be signed in to change notification settings - Fork 3
First run at everkeys.add_key #5
base: master
Are you sure you want to change the base?
Changes from 2 commits
2701368
2377fd5
cde7ceb
b3b9d85
ae59381
e5242dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
from Queue import queue, Empty | ||
|
||
import logging | ||
import threading | ||
import time | ||
|
||
import evelink | ||
import evelink.cache.sqlite | ||
|
||
from kitnirc.modular import Module | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be grouped with other third party imports |
||
|
||
import schema | ||
|
||
|
||
_log = logging.getLogger(__name__) | ||
|
||
|
||
class EveKeyError(Exception): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't appear to be used. |
||
pass | ||
|
||
|
||
class EveKeysModule(Module): | ||
"""Module to look up details on an EVE API key.""" | ||
|
||
def __init__(self, *args, **kwargs): | ||
super(EveKeysModule, self).__init__(*args, **kwargs) | ||
|
||
self.cache = evelink.cache.sqlite.SqliteCache(".evecache.sqlite3") | ||
self.results = queue() | ||
self.tasks = queue() | ||
|
||
for i in range(5): | ||
t = threading.Thread(target=self._worker) | ||
t.daemon = True | ||
t.start() | ||
|
||
self._stop = False | ||
|
||
def start(self, *args, **kwargs): | ||
super(EveKeysModule, self).start(*args, **kwargs) | ||
self._stop = False | ||
|
||
def stop(self, *args, **kwargs): | ||
super(EveKeysModule, self).stop(*args, **kwargs) | ||
self.stop = True | ||
for _ in range(10): | ||
if self.tasks.empty(): | ||
break | ||
|
||
_log.info("Evekeys still has %d threads outstanding." % | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. capitalize EveKeys identically to the module name, also this message should mention that it's part of the shutdown process |
||
self.tasks.qsize()) | ||
time.sleep(1) | ||
|
||
if not self.tasks.empty(): | ||
_log.warning("Evekeys giving up with %d threads outstanding." % | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EveKeys, shutdown process |
||
self.tasks.qsize()) | ||
|
||
def _worker(): | ||
while not self.stop: | ||
try: | ||
request = self.tasks.get(True, 5) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason to use 5 seconds here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're giving 10 seconds for everything to shut down, checking if we're asked to stop every 5 seconds doesn't seem unreasonable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but ideally we won't need to use the full 10 seconds. Modules get shut down in serial; we should avoid having to wait longer than we need to. |
||
except Empty: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see note about using fully qualified |
||
continue | ||
|
||
_add_key(request) | ||
self.tasks.task_done() | ||
|
||
def _add_key(request): | ||
keyid = request['keyid'] | ||
vcode = request['vcode'] | ||
irc_account = request['metadata']['account'].lower() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This lower() is probably redundant, but I guess it's fine to have around. |
||
|
||
try: | ||
api = evelink.api.API(api_key(keyid, vcode), cache=self.cache) | ||
account = evelink.account.Account(api=api) | ||
result = account.key_info() | ||
except evelink.APIError as e: | ||
_log.warn("Error loading API key(%s): %s" % (keyid, e)) | ||
self.results.put((request, "Failed to load api key.")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well include the key id in the message given to the user. |
||
return | ||
|
||
if result: | ||
_log.debug("key: %s, characters: %s" % (keyid, | ||
", ".join(char['name'] for char | ||
in result['characters'].itervalues()))) | ||
else: | ||
_log.warn("key: %s, invalid key.") | ||
self.results.put((request, "Invalid key.")) | ||
return | ||
|
||
try: | ||
summary = _save_key_info(keyid, | ||
vcode, | ||
irc_account, | ||
result['characters']) | ||
self.results.put((request, summary)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the DB access blew up, we shouldn't put the empty result in, instead entering an error message in the next section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but you can just put it after the entire There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can. Thought this way was more obvious from a logic flow, but will adjust. |
||
return | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No blank line here. Keep the |
||
except DatabaseError as e: | ||
_log.warn("Database error saving key(%s): %s" % (keyid, e)) | ||
self.results.put((request, "Database error, try again later.")) | ||
return | ||
|
||
def _save_key_info(keyid, vcode, irc_account, characters): | ||
session = schema.Session() | ||
|
||
irc_account = metadata['account'].lower() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The account should really get |
||
account, _ = schema.find_or_create(session, | ||
schema.Account, | ||
account=irc_account) | ||
|
||
# add key to the account | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do the key and account actually get linked? This comment seems misleading. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, that's a bug, since you're right, it doesn't actually associate it with the account, and it should. Same for the characters later. |
||
key, _ = schema.find_or_create(session, | ||
schema.ApiKey, | ||
keyid=keyid, | ||
vcode=vcode) | ||
|
||
# add characters | ||
for character in characters.itervalues(): | ||
data = { 'name': character['name'], | ||
'corp': character['corp']['name'] } | ||
char, _ = schema.find_or_create(session, | ||
schema.Character, | ||
data=data, | ||
name=character['name']) | ||
|
||
session.commit() | ||
return "%d characters added: %s" % ( | ||
len(characters), | ||
", ".join([char['name'] for char in characters])) | ||
|
||
def add_key(self, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ew, no. If this has specific args, then define it with specific args. Ideally, have it construct |
||
"""Look up a given API key, associate with account and characters. | ||
|
||
Args: | ||
keyid - API key id. | ||
vcode - API key verification. | ||
metadata['account'] - IRC account name. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really clear at all. External functions shouldn't take dicts that have random keys set. |
||
""" | ||
self.tasks.put(kwargs) | ||
|
||
|
||
# vim: set ts=4 sts=4 sw=4 et: |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,26 @@ def init_db(path): | |
Base.metadata.create_all(engine) | ||
Session.configure(bind=engine) | ||
|
||
def find_or_create(session, model, data=None, **kwargs): | ||
"""Find or create an object. | ||
|
||
returns: | ||
(possibly new) objects, | ||
was it created | ||
""" | ||
|
||
instance = session.query(model).filter_by(**kwargs).first() | ||
|
||
if instance: | ||
return instance, False | ||
else: | ||
if data: | ||
instance = model(**data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, that means you can't search using filters you don't want to use when creating the object. I think that should be fine, but it does limit a bit what we can do. |
||
else: | ||
instance = model(**kwargs) | ||
session.add(instance) | ||
return instance, True | ||
|
||
account_api_m2m = Table('account_key_m2m', Base.metadata, | ||
Column('account_pk', String, ForeignKey('account.account')), | ||
Column('api_key_pk', Integer, ForeignKey('api_key.keyid')) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's
Queue.Queue
.