-
Notifications
You must be signed in to change notification settings - Fork 42
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
add initial client skeleton #1
Conversation
According to the solution in: https://groups.google.com/forum/#!msg/sqlalchemy-alembic/dnKI0PUiMT8/Wij7EsN5BgAJ this is a reasonable approach (at least for CI). The running of alembic upgrade head will eventually go in postinst as on the securedrop server
@heartsucker you will have feelings about these choices i made, let's squabble! |
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.
Only preferential nits, no blockers.
.circleci/config.yml
Outdated
command: | | ||
virtualenv venv | ||
source venv/bin/activate | ||
pip3 install -r requirements/requirements-dev.txt |
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.
Nit: I'm more of a fan of the dev requirements being a merge of req.in
and req-dev.in
than having them be separate.
README.md
Outdated
## Run tests | ||
|
||
``` | ||
python -m unittest |
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.
y no pytest
:(
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.
mostly because I generally stick to the stdlib until I Need A Thing in a dependency, but it's a test dep and I do like fixtures so I added pytest
😛
securedrop_client/models.py
Outdated
from sqlalchemy.ext.declarative import declarative_base | ||
|
||
|
||
DB_PATH = 'svs.sqlite' |
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.
Nit: It would be nice to see a huge TODO over this to remind us to factor this out into a config for later.
Less of a nit: this should be path.abspath('svs.sqlite')
sqlite3.connect(DB_PATH) | ||
subprocess.check_output('alembic upgrade head'.split()) | ||
|
||
engine = create_engine('sqlite:///{}'.format(DB_PATH)) |
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.
I think this needs to have one more /
because DB_PATH isn't absolute. Unless sqlite does relative paths when the URI doesn't start with //
?
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.
(DB_PATH now absolute in 6dd6fa6)
And note that the config management should be handled over in #2
hey @kushaldas, let me know if you have any thoughts/suggestions on this diff, else we can merge and proceed |
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.
This looks okay in my mind. But, as we are starting this fresh, can we please pipenv
to control the dependencies?
0ce70f1
to
bef03ef
Compare
bef03ef
to
43fb4f6
Compare
fair, done! |
Adds modular version of Qubes Workstation send-to-usb script
synchronize singleton creation using threading.Lock() in __call__
made some initial choices here, pulling a PR to give people an opportunity to speak up if they don't like them 😇
alembic upgrade head
logic should eventually go inpostinst
). we want this to be incredibly easy to deploy/manage and SQLite is perfect for that, even though it does mean doing some odd migration acrobatics every now and then