Clarify docker compose file for plugin developers#83
Conversation
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
=======================================
Coverage 68.11% 68.11%
=======================================
Files 15 15
Lines 414 414
=======================================
Hits 282 282
Misses 132 132
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| EXPOSE 8080 | ||
| CMD if [ "${PATH_TO_OTEAPI_CORE}" != "/dev/null" ] && [ -n "${PATH_TO_OTEAPI_CORE}" ]; then \ | ||
| pip install -U --force-reinstall -c requirements.txt -e /oteapi_core; fi \ | ||
| pip install -U --force-reinstall -e /oteapi_core; fi \ |
There was a problem hiding this comment.
Why is requirements removed?
There was a problem hiding this comment.
It's removed as a constraint for the installation of oteapi-core. This is to avoid the installation failing, should the local version of oteapi-core differ from the one listed in requirements.txt.
Essentially, keeping the -c command here defeats the purpose of installing a local version of oteapi-core. I.e., this was really a bug.
| - otenet | ||
| volumes: | ||
| - "${PATH_TO_OTEAPI_CORE:-/dev/null}:/oteapi-core" | ||
| - "${PATH_TO_OTEAPI_ANOTHER_PLUGIN:-/dev/null}:/oteapi-another-plugin" |
There was a problem hiding this comment.
One might depend on more than one additional plugin. Wouldn't it be smarter with a single environment variable with a (e.g. colon-separated) list of plugin paths? Something like:
OTEAPI_PLUGIN_PATH="/path/to/first/plugin:/path/to/second/plugin"
Is there any reason to treat oteapi-core from other plugins with regard to this?
There was a problem hiding this comment.
OTEAPI Core is special, since it's already a core dependency of the service and is therefore set up differently within the Dockerfile.
Concerning other plugins, I don't expect anyone to install more than 1-2 external plugins to develop, other than the one they're already developing.
jesper-friis
left a comment
jesper-friis
left a comment
There was a problem hiding this comment.
Approved when the typo is fixed.
The suggestion about replacing PATH_TO_OTEAPI_CORE and `` PATH_TO_OTEAPI_ANOTHER_PLUGIN with e.g. OTEAPI_PLUGIN_PATH can be a separate issue.
3 participants
Fixes #82