Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Export architecture improvement spike #15

Closed
eloquence opened this issue Sep 19, 2019 · 4 comments · Fixed by #28
Closed

Export architecture improvement spike #15

eloquence opened this issue Sep 19, 2019 · 4 comments · Fixed by #28

Comments

@eloquence
Copy link
Member

For the 9/19-10/9 sprint, we've agreed to a time-boxed, 4 hour improvement spike to the current export tooling (I've increased the estimate to 8 total, to account for review and integration). The current architecture was a first pass, and a targeted improvement spike will likely identify several areas where it should be refactored or reorganized to increase maintainability and resilience.

This ticket exists to log initial observations, and PRs without corresponding issues can be tracked against it.

@sssoleileraaa
Copy link
Contributor

One big architecture improvement would be to return status codes (e.g. USB_CONNECTED, ERROR_USB_MOUNT, ERROR_USB_WRITE, ERROR_PRINT, ERROR_PRINTER_URI) instead of writing these codes to stderr. There are a lot of subprocess calls in the export code that sometimes results in a lot of extra text in stderr, which the client is expected to use to check status codes. For example, I've seen:

chown: cannot read directory `/media/....
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
device-mapper: remove ioctl on encrypted...
ERROR_USB_MOUNT

It would actually be useful for the client to show the ERROR_USB_MOUNT code to the user when we say to contact their admin, so a clean return code here is preferred.

@emkll
Copy link
Contributor

emkll commented Oct 17, 2019

The intent is that all exceptions should be caught and return a defined, documented error code. If there's stderr output or any other message, that means an exception that should be been caught was not caught.

The error you mention in #15 (comment) is due to the uncaught subprocess.check_call in the except block here: https://github.com/freedomofpress/securedrop-export/blob/master/securedrop_export/export.py#L242

@eloquence
Copy link
Member Author

For the 10/23-11/6 sprint period, we've agreed to finalize the round of changes made in the 15-sprint-39-improvements branch, and to land them in master.

@sssoleileraaa
Copy link
Contributor

see related discussion about device management for export

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

Successfully merging a pull request may close this issue.

3 participants