search

freedomofpress / dangerzone

Take potentially dangerous PDFs, office documents, or images and convert them to safe PDFs
https://dangerzone.rocks/
GNU Affero General Public License v3.0
3.36k stars 153 forks source link

Avoid DUMMY_CONVERSION env var treated as bool in CI #770

Closed naglis closed 2 months ago

naglis commented 2 months ago

DUMMY_CONVERSION: True is treated as a boolean value in YAML. As a result, during GitHub CI the environment variable setup during tests is formatted as DUMMY_CONVERSION=true (I suspect due to it being a Node environment?).

The value is used in tests and passed as the condition to the pytest.mark.skipif decorator. The skipif condition can be either a bool or str. When it is a str (our case, as we use os.environ.get()), it is treated as a condition string by pytest.

Since the condition string is eval()ed by pytest, trying to evaluate true results in:

Failed: Error evaluating 'skipif' condition true NameError: name 'true' is not defined

This wraps the value in single-quotes to avoid the value being treated as boolean in YAML.

I've came across it when trying to use this approach in another PR. I suspect it is not failing in its current use since the other skipif condition is checked and matches first.

apyrgio commented 2 months ago

Nice find, and thanks for the PR!

(I suspect due to it being a Node environment?).

Sigh, unfortunately not. That's baggage from YAML 1.1, which interpreted as boolean several literal values (True, Yes, On). In YAML 1.2 this is no longer the case, but lots of YAML parsers do not use that spec by default. One reason for that is that a lot of Kubernetes YAML files rely on this shortcut.

With that in mind, I think using DUMMY_CONVERSION: 1 would be less confusing. Are you ok with that? If yes, I can make this change and merge the PR.

naglis commented 2 months ago

With that in mind, I think using DUMMY_CONVERSION: 1 would be less confusing. Are you ok with that? If yes, I can make this change and merge the PR.

Sure!