-
Notifications
You must be signed in to change notification settings - Fork 33
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
Use the right config section when starting the module #62
Conversation
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.
lgtm
@@ -60,7 +60,7 @@ class S3StorageProviderBackend(StorageProvider): | |||
""" | |||
|
|||
def __init__(self, hs, config): | |||
self.cache_directory = hs.config.media_store_path | |||
self.cache_directory = hs.config.media.media_store_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.
I think we might be able to delete this entirely, its only used in store_file
as part of os.path.join
, but the path store_file
gets appears to be absolute already, so its pointless. I think.
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.
store_file
seems to be called by Synapse
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.
store_file
's path appears to be relative to me, or am I mistaken?
https://github.com/matrix-org/synapse/blob/develop/synapse/rest/media/v1/media_storage.py#L159
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 right, yes, I misread that fname
is the absolute path. Annoying.
@babolivier , I know this is after the fact, but is there value to adding a version check since this popped up in v1.45.0? That is, if someone is running < v1.45.0 would it be required to use the previous call of |
No, with Synapse < v1.45.0, reading |
@patrickstump For context we had a magic method which translated |
To be clear, is this fixing something that makes s3-storage-provider not work with newer versions of synapse? If so, what were the symptoms of the failure? Some sort of exception? |
Yes, because media storage providers apparently get passed the Synapse config directly, and v1.45.0 removed the magic that translated We should have caught this in matrix-org/synapse#10985 (comment), but forgot about the particular case of media storage providers. |
ok, thanks Brendan. As a reminder, it's very helpful to include an example of the error message, so that other people seeing the same error can search for it and find the fix. |
Ah good point. Here's the full stacktrace:
|
I upgraded synapse-s3-storage-provider together with synapse 1.45.1 using
So apparently this is not backwards compatible? Also |
@HarHarLinks Can you please open a new issue, also if you could add the output of |
No description provided.