Skip to content
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

Cloudupload nextcloud #1404

Closed
wants to merge 7 commits into from

Conversation

DavHau
Copy link

@DavHau DavHau commented Aug 9, 2019

Hey, this is my first PR here.
This adds cloud upload options for NextCloud and for other WebDav servers in general.
I developed and tested the following using docker:

  • Tesing uplaod service via 'Test Service' GUI button
  • Create missing directories automatically
  • Upload pictures and videos

While testing with docker i had the problem that i didn't have a camera connected. I wrote myself some dummy mjpeg network camera which streams cute kittens and changes their colors from time to time to simulate motion. It worked quite well for testing motioneye. Is there any better tool to use for developing? Otherwise i can add this also with another PR.

@DavHau DavHau force-pushed the cloudupload-nextcloud branch from f84db8b to 1643c00 Compare August 9, 2019 14:00
@DavHau
Copy link
Author

DavHau commented Aug 9, 2019

Now the arm pipeline takes 31 min because lxml pip package has to be compiled. Hope this is ok.

@kleini
Copy link
Collaborator

kleini commented Aug 9, 2019

While testing with docker i had the problem that i didn't have a camera connected. I wrote myself some dummy mjpeg network camera which streams cute kittens and changes their colors from time to time to simulate motion. It worked quite well for testing motioneye. Is there any better tool to use for developing? Otherwise i can add this also with another PR.

Please add your camera simulation in another PR. This will be a great help for testing.

Copy link
Collaborator

@kleini kleini left a comment

Choose a reason for hiding this comment

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

Why is travis_wait 40 needed?

.travis.yml Show resolved Hide resolved
extra/Dockerfile.armv7-armhf Outdated Show resolved Hide resolved
extra/Dockerfile.armv7-armhf Show resolved Hide resolved
Copy link
Contributor

@bob-lee bob-lee left a comment

Choose a reason for hiding this comment

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

In main.html, wonder why ‘http|https’ and ‘ftp|sftp’ were removed in two places? Code changes here should not affect other upload services anyway.


_STATE_FILE_NAME = 'uploadservices.json'
_services = None


class UploadService(object):
class UploadService:
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to test other services as UploadService and GoogleBase has changed.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by test? Unit test, or just click testing through the UI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean regression test. As base classes that existing services are based on have changed, they (existing services) need to be tested as well to make sure there was no unwanted side effect on them.

@@ -63,7 +63,7 @@ def apply_patches(self, base_dir):

packages=['motioneye'],

install_requires=['tornado>=3.1,<6', 'jinja2', 'pillow', 'pycurl'],
install_requires=['tornado>=3.1,<6', 'jinja2', 'pillow', 'pycurl', 'webdavclient3==0.12'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if webdav provides REST api so as to avoid adding webdavclient here?

Copy link
Author

Choose a reason for hiding this comment

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

Actually I used only 3 or so out of many webdav calls. Maybe the whole functionality could also be done by building the requests manually with pycurl or requests. Should i take a look into this? Still some answers of the webdav server might be in XML which has to be parsed. Also the webdav3 library handles errors nicely and throws meaningful exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a new upload service is doable without adding additional library, I would think that's the way to go to keep this project lightweight.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I certainly don't want motionEye to pull in a dep like webdavclient. This should be optional.

@DavHau
Copy link
Author

DavHau commented Aug 18, 2019

In main.html, wonder why ‘http|https’ and ‘ftp|sftp’ were removed in two places? Code changes here should not affect other upload services anyway.

First of all http/https should be removed from main.html completely in my opinion since it is nowhere used and there is no backend for it. I saw in the commit history that the http/https flags have been added with the first commits that added upload services. Probably a draft that has never been implemented.
I think those should better be removed until there is actual functionality for it. But the decision is up to you guys.
Anyway, there only was a 'Server Address' input field before my PR and the description says clearly that it takes an IP address or a domain name. A URL input is needed in my case which contains a protocol, a domain name, a port and a path all in once, so i didn't want to reuse the 'Server Address' field since its description doesn't match and i added a 'Server URL' field.
Also http and https (if they were implemented) would take a URL and not a domain, so i unlinked them from 'Server Address' which is now only used for ftp and sftp and added them also to 'Server URL'.
Should we just delete http and https from the html?

@MichaIng
Copy link
Member

MichaIng commented Mar 12, 2022

@DavHau
If you find time, could you rebase onto current dev? Generally WebDAV support makes much sense. When you do, please leave out any changes which are not directly related/required for the additional upload service(s) to work. Good fine about the unused http|https, but please do this as an own PR, so we can test this individually.

Also I agree, if this requires an additional dependency, it should be probably made optional. Either the existence of the module/function/method could be tested to display the option or not (or a note/prompt on selection that it is disabled due to missing dependency), or it could be made a config option, like SMB/CIFS is, with the additional dependency explained in the config sample.

@DavHau
Copy link
Author

DavHau commented Mar 12, 2022

I currently do not have the capacity to work on this. Also I'm not a user of motioneye anymore. Feel free to fork/copy and continue the work.

@MichaIng
Copy link
Member

MichaIng commented Mar 12, 2022

Okay, fair enough.

@jmichault @kleini @bob-lee @cclauss shall we keep this PR open, in the hope someone implements WebDAV support based on it, or shall we close it?

@MichaIng
Copy link
Member

Cross linking a similar PR with only Nextcloud upload added: #1743

@MichaIng MichaIng linked an issue Mar 13, 2022 that may be closed by this pull request
@MichaIng MichaIng linked an issue Mar 21, 2022 that may be closed by this pull request
@MichaIng
Copy link
Member

MichaIng commented May 1, 2022

All done now in #1743, marking this as closed.

@MichaIng MichaIng closed this May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Feature request: Nextcloud as a storage target webdav nexcloud
5 participants