From 06c0c7387f08afdad74575971cf331fb0511be78 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Tue, 24 Mar 2020 18:01:25 +0545 Subject: [PATCH 1/4] s3/gs: do not try to make directory on top level --- dvc/remote/gs.py | 3 +++ dvc/remote/s3.py | 3 +++ tests/func/test_s3.py | 25 +++++++++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/dvc/remote/gs.py b/dvc/remote/gs.py index 9230699535..8c6fc78fdd 100644 --- a/dvc/remote/gs.py +++ b/dvc/remote/gs.py @@ -143,6 +143,9 @@ def walk_files(self, path_info): yield path_info.replace(fname) def makedirs(self, path_info): + if not path_info.path: + return + self.gs.bucket(path_info.bucket).blob( (path_info / "").path ).upload_from_string("") diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index 61636a0154..d525cb54ae 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -238,6 +238,9 @@ def makedirs(self, path_info): # # We are not creating directory objects for every parent prefix, # as it is not required. + if not path_info.path: + return + dir_path = path_info / "" self.s3.put_object(Bucket=path_info.bucket, Key=dir_path.path, Body="") diff --git a/tests/func/test_s3.py b/tests/func/test_s3.py index 388757d8c6..3e31765e5b 100644 --- a/tests/func/test_s3.py +++ b/tests/func/test_s3.py @@ -2,6 +2,7 @@ import boto3 import moto.s3.models as s3model +import pytest from moto import mock_s3 from dvc.remote.s3 import RemoteS3 @@ -45,6 +46,30 @@ def test_copy_singlepart_preserve_etag(): RemoteS3._copy(s3, from_info, to_info, {}) +@mock_s3 +@pytest.mark.parametrize( + "base_info", + [RemoteS3.path_cls("s3://bucket/"), RemoteS3.path_cls("s3://bucket/ns/")], +) +def test_link_created_on_non_nested_path(base_info, tmp_dir, dvc, scm): + remote = RemoteS3(dvc, {"url": str(base_info.parent)}) + remote.s3.create_bucket(Bucket=base_info.bucket) + remote.s3.put_object( + Bucket=base_info.bucket, Key=(base_info / "from").path, Body="data" + ) + remote.link(base_info / "from", base_info / "to") + + assert remote.exists(base_info / "from") + assert remote.exists(base_info / "to") + + +@mock_s3 +def test_makedirs_doesnot_try_on_top_level_paths(tmp_dir, dvc, scm): + base_info = RemoteS3.path_cls("s3://bucket/") + remote = RemoteS3(dvc, {"url": str(base_info)}) + remote.makedirs(base_info) + + def _upload_multipart(s3, Bucket, Key): mpu = s3.create_multipart_upload(Bucket=Bucket, Key=Key) mpu_id = mpu["UploadId"] From d4750cbd2fdb48e9173bf3c2be26926e64d402a0 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Wed, 25 Mar 2020 15:55:49 +0545 Subject: [PATCH 2/4] tests: unittest makedirs for s3 and gs --- tests/unit/remote/test_remote.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/unit/remote/test_remote.py b/tests/unit/remote/test_remote.py index 47c38d01cf..a249e0af0c 100644 --- a/tests/unit/remote/test_remote.py +++ b/tests/unit/remote/test_remote.py @@ -1,4 +1,6 @@ -from dvc.remote import Remote +import pytest + +from dvc.remote import Remote, RemoteS3, RemoteGS def test_remote_with_checksum_jobs(dvc): @@ -25,3 +27,14 @@ def test_remote_without_checksum_jobs_default(dvc): remote = Remote(dvc, name="without_checksum_jobs") assert remote.checksum_jobs == remote.CHECKSUM_JOBS + + +@pytest.mark.parametrize("remote_cls", [RemoteGS, RemoteS3]) +def test_makedirs_not_create_for_top_level_path(remote_cls, mocker): + url = "{.scheme}://bucket/".format(remote_cls) + remote = remote_cls(None, {"url": url}) + exc = Exception("should not try to create dir on top-level path") + client = mocker.PropertyMock(side_effect=exc) + # we use remote clients with same name as scheme to interact with remote + setattr(remote_cls, remote_cls.scheme, client) + remote.makedirs(remote.path_info) From 6a2fb53fbc7172f9fdfb9266845d4ae6495bd333 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Fri, 27 Mar 2020 19:19:46 +0545 Subject: [PATCH 3/4] remove setattr and use patch.object --- tests/unit/remote/test_remote.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/unit/remote/test_remote.py b/tests/unit/remote/test_remote.py index a249e0af0c..5bcbff45a9 100644 --- a/tests/unit/remote/test_remote.py +++ b/tests/unit/remote/test_remote.py @@ -33,8 +33,10 @@ def test_remote_without_checksum_jobs_default(dvc): def test_makedirs_not_create_for_top_level_path(remote_cls, mocker): url = "{.scheme}://bucket/".format(remote_cls) remote = remote_cls(None, {"url": url}) - exc = Exception("should not try to create dir on top-level path") - client = mocker.PropertyMock(side_effect=exc) + mocked_client = mocker.PropertyMock() # we use remote clients with same name as scheme to interact with remote - setattr(remote_cls, remote_cls.scheme, client) + mocker.patch.object(remote_cls, remote.scheme, mocked_client) + remote.makedirs(remote.path_info) + + assert not mocked_client.called From 3b1168184d4342c03331f8b18f5f9cabdc618617 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Fri, 27 Mar 2020 19:24:20 +0545 Subject: [PATCH 4/4] Update tests/unit/remote/test_remote.py --- tests/unit/remote/test_remote.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/remote/test_remote.py b/tests/unit/remote/test_remote.py index 5bcbff45a9..557ab87f48 100644 --- a/tests/unit/remote/test_remote.py +++ b/tests/unit/remote/test_remote.py @@ -38,5 +38,4 @@ def test_makedirs_not_create_for_top_level_path(remote_cls, mocker): mocker.patch.object(remote_cls, remote.scheme, mocked_client) remote.makedirs(remote.path_info) - assert not mocked_client.called