UCLH-Foundry / PIXL

PIXL Image eXtraction Laboratory
Apache License 2.0
8 stars 0 forks source link

Move `pixldc` commands into PIXL CLI #411

Closed milanmlft closed 3 weeks ago

milanmlft commented 1 month ago

Fixes #392

Periphery changes:

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.04%. Comparing base (1c356f2) to head (b6ebc33).

Files Patch % Lines
cli/src/pixl_cli/_docker_commands.py 93.93% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #411 +/- ## ========================================== - Coverage 83.26% 83.04% -0.22% ========================================== Files 78 76 -2 Lines 3268 3061 -207 ========================================== - Hits 2721 2542 -179 + Misses 547 519 -28 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

milanmlft commented 4 weeks ago

Reverted back to using docker compose directly in run-system-test.sh because attempting to use pixl up/down requires the following:

There's a couple of workarounds for this, like temporarily copying over the test/.env file to project root, or loading the test/.env file through Python. But I'm going to leave it at this because this PR already took way too much time for what should have been a simple change 🫠

More importantly though, I think this points to some not-so-ideal design choices in the way we're handling these environment variables and .env files. Because when I'm running pixl down, I don't really care whether RABBITMQ_HOST is defined or not, but at the moment that will result in an error. Might be worth having a think about!