motioneye icon indicating copy to clipboard operation
motioneye copied to clipboard

Allow S3 Uploads while "Include Subdirectories" is False

Open pbouill opened this issue 1 year ago • 3 comments

Can't upload to S3 if "target_dir" is false... simplify code to get the filename using os.path -- just get the file name directly without slicing the file name string (filename[len(target_dir) :]))

Was getting:


Traceback (most recent call last):
  File "/usr/local/lib/python3.9/dist-packages/motioneye/uploadservices.py", line 1307, in upload_media_file
    service.upload_file(target_dir, filename, camera_name)
  File "/usr/local/lib/python3.9/dist-packages/motioneye/uploadservices.py", line 1227, in upload_file
    s3.upload_file(filename, self._bucket, filename[len(target_dir) :])

pbouill avatar May 15 '23 14:05 pbouill

Many thanks, makes perfectly sense. This actually makes target_dir obsolete, and using a sub directory anyway did never work with S3 as only a bucket and an object name can be passed. The switch to enable/disable upload with sub directories is however shown when selecting S3 upload, right? It then makes sense to hide the toggle like we do got Google Photos: https://github.com/motioneye-project/motioneye/blob/27971dc/motioneye/templates/main.html#L505-L509

Also here in the code, while the argument positions must not change, we can at least not assign target_dir but a dummy variable instead (is underscore common for this in Python?) and probably leave a code comment to not cause confusion among future contributors.

MichaIng avatar May 18 '23 16:05 MichaIng

I agree with Micha on also hiding the "Include subfolders" switch for S3 upload, so that we do not degrade the user experience with controls that do not cause any action or change. Otherwise, good catch and thanks for the PR.

I have no opinion regarding target_dir parameter in upload_file (camera_name was actually already not in use there, which my editor indicated readily), I'm ok with whatever practice for that that is used in Python.

zagrim avatar May 21 '23 07:05 zagrim

Some users, including myself, have the Movie File Name that includes also slashes to create subfolders automatically (for example motionEye/%Y-%m-%d/%Y-%m-%d_%H-%M-%S). However it seems that the solution proposed in this PR (using os.path.basename(filename) instead) would only keep the last part of the file name. If so it would be better to fix this before merging.

serniko97 avatar Jan 08 '24 13:01 serniko97