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

Allow S3 Uploads while "Include Subdirectories" is False #2760

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

pbouill
Copy link

@pbouill pbouill commented May 15, 2023

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) :])

@MichaIng
Copy link
Member

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.

@zagrim
Copy link
Collaborator

zagrim commented May 21, 2023

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.

@serniko97
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants