Skip to content

Implement Subpath Support for Volumes in Docker-Py (#3243) #3270

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implement Subpath Support for Volumes in Docker-Py (#3243) #3270

wants to merge 1 commit into from

Conversation

Khushiyant
Copy link
Contributor

@Khushiyant Khushiyant commented Jun 13, 2024 •
edited
Loading

Closes #3243

This PR aims to add subpath support for volumes, allowing users to specify a subpath when creating a volume or a container. This feature is essential for scenarios where a container needs to access a specific directory within a volume.

  • Updated the Mount class in docker/types/mounts.py to include a subpath attribute.
  • Added tests/integration/api_container_test.py::VolumeBindTest::test_create_with_subpath_volume_mount to verify subpath working

Refer Docker Engine v26.0 and Docker API v1.45

@Khushiyant Khushiyant changed the title Implement Subpath Support for Volumes in Docker-Py Implement Subpath Support for Volumes in Docker-Py (#3243) Jun 13, 2024
@rayrapetyan
Copy link

Hello, can this PR be merged ASAP?

@Khushiyant
Copy link
Contributor Author

@rayrapetyan Even, I'm also waiting for the review

@Khushiyant Khushiyant marked this pull request as ready for review September 16, 2024 05:13
@rayrapetyan
Copy link

@thaJeztah @krissetto, could you provide a review please, this seems like a very simple and straightforward change. Thanks!

@rayrapetyan
Copy link

@Khushiyant, could you re-trigger the build as seems one of the integration tests has failed before.

@Khushiyant
Copy link
Contributor Author

@Khushiyant, could you re-trigger the build as seems one of the integration tests has failed before.

Yeah, I triggered the build and now, all tests have passed

krissetto
krissetto reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@krissetto krissetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!
I gave this a quick look for now, we'll try to get it in soon

docker/api/volume.py Outdated
@@ -45,6 +45,9 @@ def create_volume(self, name=None, driver=None, driver_opts=None,
driver (str): Name of the driver used to create the volume
driver_opts (dict): Driver options as a key-value dictionary
labels (dict): Labels to set on the volume
subpath (str): Subpath within the volume to use as the volume's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this to

subpath (str): Path inside a volume to mount instead of the volume root.

To better match documentation we have elsewhere (eg https://github.com/docker/docs/blob/23f3fbd0639a24d0e2280fd0edb7f72b3f0cb1ca/content/reference/compose-file/services.md?plain=1#L1865)

@dvdksn WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

docker/types/services.py Outdated
@@ -242,14 +242,16 @@ class Mount(dict):
for the ``volume`` type.
driver_config (DriverConfig): Volume driver configuration. Only valid
for the ``volume`` type.
subpath (string): The path within the volume where the container's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep this the same as the suggestion above ☝️

tests/integration/api_container_test.py Outdated
mount = docker.types.Mount(
type="volume", source=helpers.random_name(),
target=self.mount_dest, read_only=True,
subpath='subdir'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick glance, it doesn't seem that the subpath is being checked in this test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes accordingly

docker/types/services.py Outdated
@@ -280,6 +282,8 @@ def __init__(self, target, source, type='volume', read_only=False,
volume_opts['Labels'] = labels
if driver_config:
volume_opts['DriverConfig'] = driver_config
if subpath:
volume_opts['SubPath'] = subpath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be written as Subpath here, right @vvoland?

Copy link
Contributor Author

@Khushiyant Khushiyant Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is Subpath instead of SubPath: moby#45687. I'll change this quick

Khushiyant
Khushiyant commented Sep 17, 2024
View reviewed changes
docker/types/services.py
@@ -280,6 +282,8 @@ def __init__(self, target, source, type='volume', read_only=False,
volume_opts['Labels'] = labels
if driver_config:
volume_opts['DriverConfig'] = driver_config
if subpath:
volume_opts['Subpath'] = subpath
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the SubPath to Subpath

krissetto
krissetto reviewed Sep 17, 2024
View reviewed changes
tests/integration/api_container_test.py Outdated
mount_data = filtered[0]
assert mount['Source'] == mount_data['Name']
assert mount_data['RW'] is False
assert mount["VolumeOptions"]["Subpath"] == "subdir"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing that the input mount options, as defined above on line 628, are the same.

The test needs to check that the path mounted inside the container corresponds to the subdir as defined in the mount options. The volume mounted must therefore contain the subdir, or else there should be an error.

Maybe you could change to check mount_data instead, if it contains the necessary info? I don't have much time now but I'd have to check how these volume tests are defined in a bit more detail later

Copy link
Contributor Author

@Khushiyant Khushiyant Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sure.

@Khushiyant Khushiyant marked this pull request as draft March 18, 2025 15:23
@Khushiyant Khushiyant force-pushed the volume-subpath branch 3 times, most recently from 6f5b179 to 64c2101 Compare March 18, 2025 17:26
@Khushiyant
Copy link
Contributor Author

Khushiyant commented Mar 18, 2025 •
edited
Loading

I have cleaned up the commit history while removing unnecessary changes and also corrected tests

@Khushiyant Khushiyant marked this pull request as ready for review March 18, 2025 17:32
@Khushiyant Khushiyant requested a review from krissetto March 18, 2025 17:33
@Khushiyant Khushiyant force-pushed the volume-subpath branch 3 times, most recently from df5eaa3 to f86cf65 Compare March 18, 2025 17:45
@Khushiyant
feat(docker/api/container): add support for subpath in volume_opts
820769e 
TESTED: Yes, added unit tests to verify subpath functionality
Signed-off-by: Khushiyant <[email protected]>
@Khushiyant Khushiyant requested a review from rayrapetyan March 18, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdksn dvdksn dvdksn left review comments

@krissetto krissetto Awaiting requested review from krissetto

@rayrapetyan rayrapetyan Awaiting requested review from rayrapetyan

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mount of volume-subpath which is available in Docker version 26.0.0, build 2ae903e using Python SDK for docker
4 participants
@Khushiyant @rayrapetyan @dvdksn @krissetto