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

Refactor storage API #375

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Refactor storage API #375

merged 4 commits into from
Mar 7, 2024

Conversation

Karolk99
Copy link
Contributor

@Karolk99 Karolk99 commented Mar 6, 2024

No description provided.

@Karolk99 Karolk99 requested a review from Rados13 March 6, 2024 16:57
@Karolk99 Karolk99 force-pushed the recording-endpoint-refactor branch from 40ed8c2 to bf34948 Compare March 6, 2024 16:59
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #375 (9798f2e) into master (a222207) will decrease coverage by 0.13%.
Report is 2 commits behind head on master.
The diff coverage is 46.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   53.31%   53.18%   -0.13%     
==========================================
  Files          61       59       -2     
  Lines        2592     2587       -5     
==========================================
- Hits         1382     1376       -6     
- Misses       1210     1211       +1     
Files Coverage Δ
recording/lib/storage/file.ex 100.00% <100.00%> (ø)
recording/lib/storage/s3.ex 94.11% <100.00%> (+0.78%) ⬆️
recording/lib/recording_endpoint.ex 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a222207...9798f2e. Read the comment docs.

recording/CHANGELOG.md Outdated Show resolved Hide resolved
recording/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should provide a way to specify a common prefix for created recording files. Because currently, recording files will be created in a specified bucket under key <recording_id>/<filename> and IMO it should be possible to specify that recording files will be stored under key recording/<recording_id>/<filename>. It would be also more symmetric with Storage.File.

@Karolk99 Karolk99 requested a review from Rados13 March 7, 2024 10:40
@@ -162,7 +162,9 @@ defmodule Membrane.RTC.RecordingEndpointTest do
setup_mock_http_request()

recording_endpoint =
create_recording_endpoint(rtc_engine, [{Storage.S3, %{credentials: @credentials}}])
create_recording_endpoint(rtc_engine, [
{Storage.S3, %{credentials: @credentials, path_prefix: ""}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if path_prefix is not provided a default value should be used which would be "" .

@Karolk99 Karolk99 merged commit 995793a into master Mar 7, 2024
24 checks passed
@Karolk99 Karolk99 deleted the recording-endpoint-refactor branch March 7, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants