-
Notifications
You must be signed in to change notification settings - Fork 22
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
Initial commit of ZenDesk attachment backup script. #9
Conversation
# Download the URLs | ||
if len(downloads) == 0: | ||
print("Error - no download URLs found in {}".format(kwargs['xml_path'])) | ||
elif not kwargs['print_urls']: |
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.
So it either prints or downloads ?
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.
Yup 👍
Line 26: The XML file is read in one go. Not scalable. The file is going to increase in size with time, so why not read it line-by-line and do the matching on lines? |
@pekrau: I was thinking that one global regex search would be better (faster) than many regexes on each line. Also, XML doesn't care about whitespace, so there could be strange linebreaks (or the entire file could be on one line). Granted, it isn't, but I didn't know that when I wrote the code :) Do you think that the XML will grow to such a size that it won't fit in memory? |
I should probably mention - the "proper" way to do this of course is to parse the XML. Then we could get extra data about each attachment, such as ticket ID, date and so on. This was a quick and dirty script to just get something in place so I avoided that stuff. Also it could be argued that a simpler approach is more robust to changes in the ZenDesk XML format. |
I just thought of one update which could be useful - checking that we don't already have a file of that filename before downloading. This would make subsequent backups much faster. However, not sure how they will be run - will it be on a blank location every time? |
I don't think XML allows whitespace breaks in string value of type URL? So that is not a concern. The file being one line: Could happen. But then the Zendesk programmers would be idiots... XML file size: Well, the size will keep growing indefinitely, but maybe not faster than memory gets cheaper... But I think in general it is good to assume that a file one doesn't know the size of could become huge, and use the appropriate programming pattern. But that's me being old-fashioned and paranoid. I remember the days when 2 MB in RAM was considered utopian... Proper parsing: I thought about that also, but I don't think it's worth the hassle, since your pattern will handle it. |
Checking if file exists: Yes, please do! Do you mean blank target location (disk on our machine)? Not sure. Maybe we should check with what IT Support considers to be a good policy, but why not just dump it all into one directory, which will grow with new files for each download? |
@pekrau: Skipping existing files done. Mind if I'm lazy about the other point? I'll volunteer to re-write if it becomes a problem 😀 |
OK, OK. I will hunt you down when the script goes haywire in 20 years time... |
Initial commit of ZenDesk attachment backup script.
Deal :) Thanks! |
No description provided.