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.

Sign up for GitHub

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

Jump to bottom

Abstract Isolation Providers and Add Dummy One #302

Merged
merged 8 commits into from
Jan 25, 2023
Merged

Abstract Isolation Providers and Add Dummy One #302

merged 8 commits into from
Jan 25, 2023

Conversation

deeplow
Copy link
Contributor

@deeplow deeplow commented Jan 4, 2023

Encapsulates isolation providers (will be useful for adding Qubes support later) and adds a dummy one. It can be run on both the CLI and GUI with the --unsafe-dummy-conversion flag (hidden and available only in the testing environment). This is a step for adding Windows and MacOS to the CI testing environment, given that this way (at the cost of some fidelity) we gain the ability to run automated tests without the need for nested virtualization.

$ ./dev_scripts/dangerzone-cli --unsafe-dummy-conversion tests/test_docs/sample.gif

It runs as expected but doesn't actually do any conversion. Instead it shows preset output conversion progress messages and always "succeeds" in the conversion.

╭──────────────────────────╮
│           ▄██▄           │
│          ██████          │
│         ███▀▀▀██         │
│        ███   ████        │
│       ███   ██████       │
│      ███   ▀▀▀▀████      │
│     ███████  ▄██████     │
│    ███████ ▄█████████    │
│   ████████████████████   │
│    ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀    │
│                          │
│    Dangerzone v0.4.0     │
│ https://dangerzone.rocks │
╰──────────────────────────╯
Assigning ID 'Mw9Eqs' to doc '/home/user/dangerzone/tests/test_docs/sample.gif'

Converting document to safe PDF
Marking doc Mw9Eqs as 'converting'
Dummy converter started:
  - document: sample.gif (Mw9Eqs)
  - ocr     : None

(simulating conversion)
[doc Mw9Eqs] 0.0% Converting to PDF using GraphicsMagick
[doc Mw9Eqs] 3.0% Separating document into pages
[doc Mw9Eqs] 5.0% Converting page 1/1 to pixels
[doc Mw9Eqs] 50.0% Converted document to pixels
[doc Mw9Eqs] 50.0% Converting page 1/1 from pixels to PDF
[doc Mw9Eqs] 95.0% Merging 1 pages into a single PDF
[doc Mw9Eqs] 97.0% Compressing PDF
[doc Mw9Eqs] 100.0% Safe PDF created
Marking doc Mw9Eqs as 'safe'

Safe PDF(s) created successfully
/home/user/dangerzone/tests/test_docs/sample-safe.pdf

Note: No actual 'safe' PDF is created (as opposted to what the last line says).

@deeplow deeplow added this to the 0.4.1 milestone Jan 4, 2023
@deeplow deeplow requested a review from apyrgio January 4, 2023 12:33
@deeplow deeplow mentioned this pull request Jan 4, 2023
Merged
@apyrgio
Copy link
Contributor

apyrgio commented Jan 12, 2023 •
edited
Loading

Minor commit message issue: In Isolation-provider-specific methods in _convert(), you're missing a verb ("All isolation providers will some similar steps ...")

apyrgio
apyrgio reviewed Jan 12, 2023
View reviewed changes
Copy link
Contributor

@apyrgio apyrgio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@deeplow I just finished my first pass of the PR. Feel free to share your thoughts.

dangerzone/gui/main_window.py Outdated Show resolved Hide resolved
dangerzone/isolation_provider.py Outdated Show resolved Hide resolved
@deeplow deeplow force-pushed the 229-isolation-provider branch 5 times, most recently from f961895 to 61481a0 Compare January 18, 2023 10:39
@deeplow
Copy link
Contributor Author

deeplow commented Jan 18, 2023

I have now revered the changes and save a dummy PDF so all tests pass on platforms where the dummy converter will be enabled.

@apyrgio
Copy link
Contributor

apyrgio commented Jan 18, 2023

I checked your fixups and they look good. I also tested the code a bit locally. Feel free to merge!

@deeplow deeplow mentioned this pull request Jan 20, 2023
Merged
@deeplow
Copy link
Contributor Author

deeplow commented Jan 20, 2023

Rebased on top of main and added one last FIXUP commit which made it to wrong branch.

@apyrgio
Copy link
Contributor

apyrgio commented Jan 23, 2023

I saw the FIXUP commit. I'm ok with the static method, but just wanted to mention that you are free to use self. for static methods. You don't need to refer to the class by name.

deeplow added a commit that referenced this pull request Jan 23, 2023
@deeplow
FIXUP: Move isolation_providers/__init__.py to base.py
fd9bd37 
In general, it's preferable to have a base.py module, and import that.
The reason is that you may want to re-export the isolation provider
classes in __init__.py, and doing so would lead to cyclic references.[1]

[1]: #302 (comment)
@deeplow
Copy link
Contributor Author

deeplow commented Jan 24, 2023

just wanted to mention that you are free to use self. for static methods. You don't need to refer to the class by name.

I wish I could, but because I am calling this already from a static context, I don't have access to self.

@deeplow
Rename container.py to isolation_provider.py
1114a0d 
First step in encapsulating the isolation provider.
@deeplow
Abstract container into an IsolationProvider
a4f27af 
Encapsulate container logic into an implementation of
AbstractIsolationProvider. This flexibility will allow for other types
of isolation managers, such as a Dummy one.
@deeplow
Isolation-provider-specific methods in _convert()
7ed1fd6 
All isolation providers will some similar steps when convert() is
called. For this reason, all the common parts are captured in convert()
and then each isolation provider implements its own specific conversion
process in _convert() (which is called from the convert() method).
@deeplow
Split isolation providers into their own .py files
538df18 
Provides more clear code organization having each provider in their own
python file rather than a single one.
@deeplow
Add dummy isolation provider to CLI
da0cb6b 
When enabled, the conversion part does nothing but print some simulated
output. This can be useful for testing non-conversion code (e.g. GUI).

Activated with the hidden flag --unsafe-dummy-conversion.
@deeplow
Add dummy conversion to GUI
a339eff
@deeplow
De-duplicate print_progress() logic
f5c4847
@deeplow
Make container-specific methods static
724dd2a 
Make these methods callable without having to create an instance of the
Container class. This was needed to make pytest-wrapper.py cleaner.
@deeplow
Copy link
Contributor Author

deeplow commented Jan 25, 2023

Rebased from main and squashed commits.

@deeplow
Copy link
Contributor Author

deeplow commented Jan 25, 2023

Fedora-36 failed but I'll merge it anyways since it appears that the issue is not related to the PR.

@deeplow deeplow merged commit 724dd2a into main Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apyrgio apyrgio apyrgio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@deeplow @apyrgio