Refactor parsing of extra mounts #199
Conversation
danielhollas
force-pushed
the
refactor-mount-parsing
branch
2 times, most recently
from
89dd9aa
to
d54031d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 85.85% 85.93% +0.07%
==========================================
Files 9 9
Lines 919 924 +5
==========================================
+ Hits 789 794 +5
Misses 130 130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
danielhollas
force-pushed
the
refactor-mount-parsing
branch
from
94c24b3
to
7392d02
Compare
danielhollas
force-pushed
the
refactor-mount-parsing
branch
from
f53cad5
to
c484c4b
Compare
danielhollas
changed the title
| @@ -142,8 +142,11 @@ def instance(docker_client, profile): | |||
|
|
|||
| def remove_extra_mounts(): | |||
| for extra_mount in instance.profile.extra_mounts: | |||
| extra_volume, _, _ = instance.profile.parse_extra_mount(extra_mount) | |||
| docker_client.volumes.get(str(extra_volume)).remove() | |||
There was a problem hiding this comment.
This was a bug in the test suite - we were trying to remove bind mounts as if they were volumes, see the new code below.
|
Not sure it is related or not, @edan-bainglass mentioned he encountered an issue that when an extra mount path was deleted, the |
|
Yes, the current behaviour is that the path to the extra bind mount must exist, otherwise we through the error. The assumption was that this prevents user mistakes. If we want to change that behaviour, @edan-bainglass please submit an issue on this repo. I'd be keen to learn about how you are using the extra mounts (my original implementation was driven by the need to have a closed-source QM program available inside the container). |
I noticed we can reuse the
dockerAPI logic when parsing the mount strings (such as/home/user/extra_mount/:/opt/:rw). Interestingly, the API is more forgiving than we were so we keep the extra validation. Overall the goal of this refactor was to make the code a bit more robust.This PR was motivated by a bug in a test suite that I found (and fixed here as well).