Add support for recursive up- and downloads #64

stefanseefeld · 2020-09-01T19:31:32Z

Description

This PR allows the art.artifacts.deploy() and art.artifacts.download() methods to operate on directories recursively.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
(one could argue that the existing docs don't explicitly talk about arguments types (file or directory),
would it should at least be in the release notes)

How has it been tested ?

I ran all existing tests (obviously), and added a new test (test_artifacts.py::test_download_folder_success)
to make sure downloading a directory work.

I did not add a new test to verify directory uploads.

Checklist:

My PR is ready for prime time! Otherwise use the "Draft PR" feature
All commits have a correct title
Readme has been updated
Quality tests are green (see Codacy)
Automated tests are green (see pipeline)

stefanseefeld · 2020-09-01T19:33:16Z

tests/test_artifacts.py

-    "repo": ARTIFACT_FOLDER,
-    "path": ARTIFACT_PATH,
+    "repo": ARTIFACT_REPO,
+    "path": ARTIFACT_SHORT_PATH,


The above line actually fixes a minor bug, as previously the path would include the repo, which is wrong.

anancarv · 2020-09-02T09:13:19Z

pyartifactory/objects.py

+        else:
+            if topdown:
+                yield info
+            for subdir in [c for c in info.children if c.folder is True]:


Can you please replace replace the variable c by child for more readibility ?

anancarv · 2020-09-02T09:13:37Z

pyartifactory/objects.py

+                yield from self._walk(
+                    artifact_path + subdir.uri, topdown=topdown, onerror=onerror
+                )
+            for file in [c for c in info.children if c.folder is not True]:


anancarv · 2020-09-02T09:16:09Z

pyartifactory/objects.py

+                logger.debug("Artifact %s successfully deployed", local_filename)
+        return self.info(artifact_path)
+
+    def _download(self, artifact_path: str, local_directory_path: str = None) -> str:


Can you add a docstring for this method. Indeed, according to PEP 8:

Docstrings are not necessary for non-public methods, but you should have a comment that describes what the method does. This comment should appear after the "def" line.

I have cloned the docstring ffrom download and adjusted both slightly.

anancarv · 2020-09-02T09:20:09Z

Hi @stefanseefeld
First of all, thanks for your contribution. It's a great feature !!!
I only requested a few changes, but everything seems good to me. Once they're addressed, your PR is good for merging. I also added @nymous for having a second review. Thanks again

nymous · 2020-09-04T20:37:40Z

Thanks @stefanseefeld for this really quick contribution!

I haven't fully dived in the code yet but I found an issue: the local_directory_path in the download() method is supposed to be optional, but when calling art.artifacts.download("my-repo") the following stacktrace happens:

Traceback (most recent call last):
  File "main.py", line 3, in <module>
    a.artifacts.download("generic-adeo-software-factory")
  File "/tmp/tmp.jBQC1mA3gL/.venv/src/pyartifactory/pyartifactory/objects.py", line 869, in download
    local_path = join(local_directory_path, full_path[len(prefix) :])  # type: ignore[arg-type]
  File "/usr/lib/python3.8/posixpath.py", line 76, in join
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

My first impression is that it comes from https://github.com/anancarv/python-artifactory/pull/64/files#diff-273e848550ac13f5a53c2123755a008aR869, where you use local_directory_path instead of local_file_full_path (but again it's late here ^^'). Also it's picked up correctly by mypy, but you silenced the error with the # type: ignore ^^ (mypy detects that local_directory_path can be optional).

Also, the commented assert could be removed on the line above.

I will read the code more thoroughly tomorrow after some good sleep!

stefanseefeld · 2020-09-05T21:23:26Z

Thanks @nymous for your thorough review and test. You are entirely right ! I had confused myself and complicated the code. As you can see from my follow-up commit, I addressed the problem by changing the local_directory_path default value from None to ".", then removed some obsolete lines.

nymous

Awesome! Sorry for the delay ^^'
You fixed the bug I found! I still have a few remarks, but after that it's good to go!

nymous · 2020-09-07T17:43:50Z

pyartifactory/objects.py

@@ -752,6 +754,32 @@ def delete(self, permission_name: str) -> None:
 class ArtifactoryArtifact(ArtifactoryObject):
    """Models an artifactory artifact."""

+    def _walk(
+        self, artifact_path: str, topdown: bool = True, onerror=None


What purpose does the onerror parameter serve? I don't see it used in the rest of the code.

Ah, I see you copied the signature of the os.walk(). But even then, if this callback isn't called in the implementation I don't think it will have any effect.

You are right, I cloned it from os.walk() even though at this time it isn't being used. I was hesitating whether or not to keep it, but opted to leave it in should we ever want to need it. (For example, it might be useful at some point to add more test cases, where some files could trigger errors during attempt to download them.)
Either way, I'm not hung up on it, so if you guys would prefer not to have the unused parameter there, I can certainly remove it.

nymous · 2020-09-07T17:45:20Z

pyartifactory/objects.py

+            for subdir in [child for child in info.children if child.folder is True]:
+                yield from self._walk(
+                    artifact_path + subdir.uri, topdown=topdown, onerror=onerror
+                )
+            for file in [child for child in info.children if child.folder is not True]:


A bit nitpicky here, but do these work with generator comprehensions instead of list comprehensions? Something like for subdir in (child for child in info.children if child.folder is True):

Sure, I can do that.

nymous · 2020-09-07T18:26:51Z

pyartifactory/objects.py

+            with open(local_file_location, "rb") as file:  # type: ignore[assignment]
+                self._put(f"{artifact_path}", data=file)


I think I would rather change the variable name from file to something else than ignore a mypy error. Here mypy is confused because there is already a file variable declared above (in the for loop) with the type str, so it doesn't want you to reassign it to a BinaryIO type.
Something like this fixes it (but you can choose another name if you want):

Suggested change

with open(local_file_location, "rb") as file: # type: ignore[assignment]

self._put(f"{artifact_path}", data=file)

with open(local_file_location, "rb") as file_to_upload:

self._put(f"{artifact_path}", data=file_to_upload)

Here I actually disagree: there are actually two distinct variables (within non-overlapping scopes), so it's actually a shortcoming of the mypy tool not to support this -yet. (There are discussions to improve that, see python/mypy#6232 for example.)

Oh, I see. Too bad about mypy ^^'
I don't know how to handle it, there is a workaround to avoid bypassing the error but it's annoying. I'm afraid we will forget it there and silence too many errors without knowing it.

I can add a comment to explain the issue. (But I do have a rather strong aversion against changing perfectly fine code just to make some tool happy ;-) )

nymous · 2020-09-07T18:36:59Z

pyartifactory/objects.py

        :param artifact_path: Path to file in Artifactory
        :param local_file_location: Location of the file to deploy


Maybe update this documentation to indicate that it can also accept a folder?
Like this?

Suggested change

:param artifact_path: Path to file in Artifactory

:param local_file_location: Location of the file to deploy

Deploy an artifact or a folder to an Artifactory repository.

If deploying a file, the artifact_path MUST end with the destination artifact name.

:param local_file_location: Location of the file or folder to deploy

:param artifact_path: Path to the artifact in Artifactory

nymous · 2020-09-07T18:46:26Z

pyartifactory/objects.py

+            full_path = art.repo + art.path
+            local_path = join(local_directory_path, full_path[len(prefix) :])
+            if isinstance(art, ArtifactFolderInfoResponse):
+                os.mkdir(local_path)


Would it be safer to use os.makedirs(local_path, exist_ok=True)? That way we can keep downloading in the same folder every time.

I'm not sure I understand. The walk's topdown flag guarantees that missing parent directories will be created first, so I don't see where os.mkdir() would fail where os.makedirs() wouldn't. Or at least, if it would fail, it would already fail prior to my PR, so I didn't add any implicit pre-conditions. :-)

The issue isn't about non-existing folders, it's about already existing ones ^^
If you download a folder again for example, the folder structure is already there and os.mkdir errors if the directory already exists.

Oh, indeed, good point ! :-)

nymous

With this comment about mypy I approve!
@anancarv you can merge whenever you want 😉

Fix artifact path in test.

a521831

stefanseefeld commented Sep 1, 2020

View reviewed changes

anancarv requested review from nymous and anancarv September 2, 2020 07:34

anancarv requested changes Sep 2, 2020

View reviewed changes

anancarv added this to In progress in Kanban board Sep 2, 2020

Add support for recursive downloads and uploads.

33316ac

stefanseefeld force-pushed the recursive branch from 9d06794 to 33316ac Compare September 2, 2020 13:17

anancarv previously approved these changes Sep 2, 2020

View reviewed changes

Address review comments.

4af7229

stefanseefeld dismissed anancarv’s stale review via 4af7229 September 5, 2020 21:20

nymous requested changes Sep 7, 2020

View reviewed changes

Address further review comments.

769b204

nymous approved these changes Sep 7, 2020

View reviewed changes

anancarv merged commit 02f263f into anancarv:master Sep 7, 2020

Kanban board automation moved this from In progress to Done Sep 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for recursive up- and downloads #64

Add support for recursive up- and downloads #64

stefanseefeld commented Sep 1, 2020

stefanseefeld Sep 1, 2020

anancarv Sep 2, 2020

stefanseefeld Sep 2, 2020

anancarv Sep 2, 2020

stefanseefeld Sep 2, 2020

anancarv Sep 2, 2020

stefanseefeld Sep 2, 2020

anancarv commented Sep 2, 2020

nymous commented Sep 4, 2020

stefanseefeld commented Sep 5, 2020

nymous left a comment

nymous Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous Sep 7, 2020

stefanseefeld Sep 7, 2020

nymous left a comment

		with open(local_file_location, "rb") as file: # type: ignore[assignment]
		self._put(f"{artifact_path}", data=file)

		:param artifact_path: Path to file in Artifactory
		:param local_file_location: Location of the file to deploy

-        :param artifact_path: Path to file in Artifactory
-        :param local_file_location: Location of the file to deploy
+        Deploy an artifact or a folder to an Artifactory repository.
+        If deploying a file, the artifact_path MUST end with the destination artifact name.
+        :param local_file_location: Location of the file or folder to deploy
+        :param artifact_path: Path to the artifact in Artifactory

Add support for recursive up- and downloads #64

Add support for recursive up- and downloads #64

Conversation

stefanseefeld commented Sep 1, 2020

Description

Type of change

How has it been tested ?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anancarv commented Sep 2, 2020

nymous commented Sep 4, 2020

stefanseefeld commented Sep 5, 2020

nymous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nymous left a comment

Choose a reason for hiding this comment