-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add support for recursive up- and downloads #64
Conversation
"repo": ARTIFACT_FOLDER, | ||
"path": ARTIFACT_PATH, | ||
"repo": ARTIFACT_REPO, | ||
"path": ARTIFACT_SHORT_PATH, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above line actually fixes a minor bug, as previously the path
would include the repo, which is wrong.
pyartifactory/objects.py
Outdated
else: | ||
if topdown: | ||
yield info | ||
for subdir in [c for c in info.children if c.folder is True]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please replace replace the variable c by child
for more readibility ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pyartifactory/objects.py
Outdated
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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have cloned the docstring ffrom download
and adjusted both slightly.
Hi @stefanseefeld |
9d06794
to
33316ac
Compare
Thanks @stefanseefeld for this really quick contribution! I haven't fully dived in the code yet but I found an issue: the
My first impression is that it comes from https://github.com/anancarv/python-artifactory/pull/64/files#diff-273e848550ac13f5a53c2123755a008aR869, where you use Also, the commented I will read the code more thoroughly tomorrow after some good sleep! |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
pyartifactory/objects.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What purpose does the onerror
parameter serve? I don't see it used in the rest of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pyartifactory/objects.py
Outdated
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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that.
with open(local_file_location, "rb") as file: # type: ignore[assignment] | ||
self._put(f"{artifact_path}", data=file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;-) )
pyartifactory/objects.py
Outdated
:param artifact_path: Path to file in Artifactory | ||
:param local_file_location: Location of the file to deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update this documentation to indicate that it can also accept a folder?
Like this?
: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 |
pyartifactory/objects.py
Outdated
full_path = art.repo + art.path | ||
local_path = join(local_directory_path, full_path[len(prefix) :]) | ||
if isinstance(art, ArtifactFolderInfoResponse): | ||
os.mkdir(local_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, indeed, good point ! :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this comment about mypy I approve!
@anancarv you can merge whenever you want 😉
Description
This PR allows the
art.artifacts.deploy()
andart.artifacts.download()
methods to operate on directories recursively.Type of change
Please delete options that are not relevant.
(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: