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

Pluggable signature storage #71

Merged
merged 12 commits into from Jan 25, 2017
Merged

Conversation

takluyver
Copy link
Member

This makes signature storage pluggable at NotebookNotary.store, and provides two implementations:

  • SQLiteSignatureStore is the default
  • MemorySignatureStore is a fallback and an example for other implementations.

I'm experimenting with configuring the store as an instance rather than the factory pattern we use elsewhere. This relies on using Python config files, and means that configuration would look like this:

from sqlsigs import MySQLSignatureStore
c.NotebookNotary.store = MySQLSignatureStore('127.0.0.1', database='nbsignatures')

# Rather than:
c.NotebookNotary.store_type = 'sqlsigs.MySQLSignatureStore'
c.MySQLSignatureStore.server = '127.0.0.1'
c.MySQLSignatureStore.database = 'nbsignatures'

@@ -107,7 +286,16 @@ def _data_dir_default(self):
app = JupyterApp()
app.initialize(argv=[])
return app.data_dir


store = Instance(SignatureStore)
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be configurable, if users are to set it in config files

@minrk
Copy link
Member

minrk commented Nov 30, 2016

Awesome! Looks very nice. 👍 on the configurable Instance idea vs the factory approach.

@minrk
Copy link
Member

minrk commented Nov 30, 2016

Are you okay holding this for 4.3? I'd like to push out 4.2 with the json output stuff that's ready to go.

@takluyver takluyver added this to the 4.3 milestone Nov 30, 2016
@takluyver
Copy link
Member Author

Yep, no problem with holding this for 4.3.

@minrk
Copy link
Member

minrk commented Dec 1, 2016

Do you want to add a test to exercise notary loading a store instance from config?

@takluyver
Copy link
Member Author

I have added such a test, but it highlighted something unexpected: traitlets deepcopy-s config values before they're applied, so the store is actually a copy of the one you create in config. I can't immediately see a problem with that, but it seems like the kind of weird thing that could bite someone and be really hard to figure out. So I'm wondering about reverting to the usual factory pattern configurable.

@takluyver
Copy link
Member Author

Finally got round to looking at this again. I have switched the configuration to a more conventional factory-pattern approach to avoid the deepcopy issue.

@minrk
Copy link
Member

minrk commented Jan 25, 2017

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants