Added CLI args and set inject home directory #43
Conversation
There was a problem hiding this comment.
thanks for the PR @heartsucker! just a couple of notes inline
| """ | ||
| All logging related settings are set up by this function. | ||
| """ | ||
| if not os.path.exists(LOG_DIR): |
There was a problem hiding this comment.
Why remove the log directory?
There was a problem hiding this comment.
I was hoping to keep the directory logic all in one place so I pulled that out for now. I can readd it but...
I guess this brings up a broader question which is how strict do we want to be on data directory permissions? Like I think every dir needs to have 00 for the final two bytes, and we should bail otherwise. I know it's Qubes already, but if the directories don't need global rwx, then it shouldn't be set.
There was a problem hiding this comment.
let's keep the logs dir to prevent clutter
agreed on permissions
securedrop_client/app.py
Outdated
| description='SecureDrop Journalist GUI') | ||
| parser.add_argument( | ||
| '-H', '--home', default=DEFAULT_HOME, | ||
| help='Home directory for storing files and state') |
There was a problem hiding this comment.
let's avoid the term home for clarity (this comment applies to the entire diff unless we are actually talking about regular ~ home dir), i.e.:
home -> data_dir (or something similar)
"Home directory for storing files and state" -> "Directory for storing securedrop-client files"
There was a problem hiding this comment.
Ok, then I propose we call it sdc_home or client_home to mimic --gpg-home. I think data_dir isn't totally accurate because to me logs are configs aren't "data" in the /var/lib sense that the term "data dir" often gets used with.
There was a problem hiding this comment.
I'm cool with either of these names tbh
run.sh
Outdated
| Base.metadata.create_all(make_engine("$HOME")) | ||
| EOF | ||
|
|
||
| exec python -m securedrop_client --home "$HOME" |
There was a problem hiding this comment.
so the developer workflow is:
on first run or any run where we want a temporary database for development:
./run.sh
And developers that want to use a persistent database can do:
python -m securedrop_client --home dirwhereikeepmysvsdatabase/
if you agree, want to add a note to the README.md indicating the above?
@ntoll does this jive with your workflow? 😄
There was a problem hiding this comment.
looks like this wasn't addressed? we should definitely add this information to the readme in the getting started section as it's non obvious unless you read through the dev env
heartsucker
force-pushed
the
cli-args
branch
5 times, most recently
from
e0f8002 to
5ca62a8
Compare
|
OK... I think GitHub is working now... on with the PR-mageddon. ;-) This looks great although Question: how puritanical do we want to be about test coverage? I tend to think that if we can write a test to exercise some code that isn't already exercised, then we should. Furthermore, if we have problems writing tests for some of the code we've written, then we've probably written it wrong. Finally, if there are places (and, let's be honest, there usually are) where coverage checking isn't really needed we ought to explicitly suppress it so furture-selves / others can see our decision. Having said all that, I'm not that puritanical about this sort of thing, open and flexible to hear what folks think. I guess the important thing is we come to a consensus. :-) |
|
Ughhhhhhhhhhhh |
|
@heartsucker hugs |
Rename sd-journalist to sd-proxy
Fixes #37
Fixes #38
Fixes #39
Adds an arg parser and makes everything injectable. There is no longer shared global state.