Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Puts API keys in the database #114

Merged
merged 5 commits into from
Feb 11, 2016
Merged

Puts API keys in the database #114

merged 5 commits into from
Feb 11, 2016

Conversation

mmulich
Copy link
Contributor

@mmulich mmulich commented Jan 27, 2016

This adds Beaker as a dependency for caching the results of a function. It's only usage at this time is hardcoded to cache for 24 hours. Later we can add a means of invalidation to our admin interface, but for now one day shouldn't be a problem, especially since no one else is using publishing.

The thing to test here is the db-migrator step. One, does the migration directory get found? And two, can you apply the migration and roll it back?

@@ -40,6 +40,7 @@


__all__ = (
'db_connect', 'with_db_cursor',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with_db_cursor got removed :p

@karenc
Copy link
Contributor

karenc commented Jan 27, 2016

Looks good! Do we need a data migration to create the initial api keys?

@mmulich mmulich force-pushed the api-keys-in-db branch 2 times, most recently from ed1a2fd to be17183 Compare January 30, 2016 03:10
@mmulich
Copy link
Contributor Author

mmulich commented Feb 4, 2016

@karenc, there is no need to migrate keys. The keys within the configuration are plastic teething keys that were never meant to be used in production anyway.

@reedstrm
Copy link
Contributor

reedstrm commented Feb 8, 2016

I like the db connect refactoring, and moving to per-user api keys, of course, is wonderful. The caching is necessary, I understand, since looking up the api key for a user will happen on pretty much every request. We need a big FIXME for when we implement the admin side (assigning keys to users, or users to particular groups) to be sure we invalidate the cache appropriately.

This moves the api-key configuration from the configuration file
into the database, which should allow the addition of api-keys without
having to restart the service.
@mmulich mmulich force-pushed the api-keys-in-db branch 2 times, most recently from 715fc8c to 48e8675 Compare February 8, 2016 22:37
@mmulich
Copy link
Contributor Author

mmulich commented Feb 8, 2016

@reedstrm See if the latest commit meets your needs.

🐣 This also adds a means for invalidating the cache.
@reedstrm
Copy link
Contributor

yup, that covers the minimal case. BTW, why beaker, rather than the memcached solution we used over in archive? I think the answer is - we don't expect to run more than one instance of publishing, and since this isn't python3, we can't just use functools.lru_cache ?

@mmulich
Copy link
Contributor Author

mmulich commented Feb 10, 2016

I choose Beaker because it is a scalable solution. For the time being and in development, we can use memory caching. Later we can configure it to use memcached (or some other cache service). I actually plan to change cnx-archive to use Beaker sometime in the near future. Essentially, it's one less thing we developed that just works, abstracts the problem and has documentation.

reedstrm added a commit that referenced this pull request Feb 11, 2016
@reedstrm reedstrm merged commit fa81082 into master Feb 11, 2016
@reedstrm reedstrm deleted the api-keys-in-db branch February 11, 2016 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants