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.

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

Volume Mount for Docker Nodes isn't working according to docstring #31

Open
lhoff94 opened this issue Jun 13, 2022 · 1 comment
Open

Comments

@lhoff94
Copy link

lhoff94 commented Jun 13, 2022

In node/docker.py the docstring describes the volume parameter as follows:

volumes : list of dict or list of str
        A dictionary of volumes. Each entry has a name or (absolute) path as key
        and settings or a absolute path inside the container as value. See :code:`examples/volumes_and_ports.py`.

There are multiple issues with:

  1. The referenced example is no longer existing. The example was deleted with the following commit. I'd suggest to restore this or expand the docstring or documentation
  2. The docstring defines the parameter to be either a list of dict or a list of str but during the init function the referencing statement expects it to be a dict since its using the items() function like this:
self.volumes = dict(map(expand_volume_shorthand, volumes.items())) if volumes else None

using the expand_volume_shorthand function.
I checked with the documentation of the docker python library and i don't really understand the purpose of the function other then to provide default for the mounting mode. The library accepts lists as well as dicts and i can't see a reason to omit the custom function and directly relay the parameter to the docker lib.

If there are no objections, i would create a PR that gets rid of the "expand_volume_shorthand" function and parsing of the parameter. Instead I would pass through the parameter and adapt the docstring accordingly.

@lhoff94
Copy link
Author

lhoff94 commented Jun 13, 2022

Ok i guess i found the reason why the volumes_and_ports example got deleted. It doesn't really work with Marvis running inside a docker while being connected to the docker daemon on the host.
The example uses os.path.dirname to get the directory of the scenario file and uses that to build a absolute path. But all of that happens inside the container and then the absolute path gets passed to the docker daemon running outside of the container.

This feels like it should be documented as a limitation of running marvis inside a docker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant