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

api: FIx object store URL parsing #1228

Merged
merged 13 commits into from
Aug 25, 2022
Merged

Conversation

victorges
Copy link
Member

@victorges victorges commented Aug 23, 2022

What does this pull request do? Explain your changes. (required)
Fixes the API logic for parsing Object Store URLs, currently used only for the
VOD APIs for uploading files (both direct and tus).

The fix is necessary because the Go library for parsing these URLs changed
in an incompatible way for the Catalyst VOD work to flow through. Specifically, it does
not support having regions in the URLs anymore because it now supports specifying a path
prefix after the bucket name.

Specific updates (required)

  • Create a helper function to parse these URLs so we don't need to keep repeating the logic
  • Update it to allow URLs to be specified without the region
  • Fix and add some tests

-

  • How did you test each of these updates (required)
  • yarn test
  • Deploy on staging, update OS URLs and ensure upload works

Does this pull request close any open issues?
Implements #1205

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@victorges victorges requested a review from a team as a code owner August 23, 2022 20:54
@vercel
Copy link

vercel bot commented Aug 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-studio ✅ Ready (Inspect) Visit Preview Aug 25, 2022 at 7:32PM (UTC)

@victorges victorges changed the title Vg/feat/fix os url parsing api: FIx object store URL parsing Aug 23, 2022
Easier than doing that on the DB
Copy link
Member

@gioelecerati gioelecerati left a comment

Choose a reason for hiding this comment

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

For some reason tests are failing for me on playback, asset and auth

const opts: tus.S3StoreOptions | S3ClientConfig = {
...s3config,
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@gioelecerati gioelecerati left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -45,7 +45,7 @@ describe("controllers/playback", () => {
beforeEach(async () => {
await db.objectStore.create({
id: "mock_vod_store",
url: "http://user:password@localhost:8080/us-east-1/vod",
url: "s3+http://user:password@localhost:8080/us-east-1/vod",
Copy link
Member

Choose a reason for hiding this comment

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

ooooh I see

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #1228 (e9395c3) into master (bb2110f) will increase coverage by 0.14938%.
The diff coverage is 69.23077%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1228         +/-   ##
===================================================
+ Coverage   51.39369%   51.54307%   +0.14938%     
===================================================
  Files             66          66                 
  Lines           4341        4342          +1     
  Branches         781         783          +2     
===================================================
+ Hits            2231        2238          +7     
+ Misses          1830        1825          -5     
+ Partials         280         279          -1     
Impacted Files Coverage Δ
packages/api/src/controllers/asset.ts 56.27010% <0.00000%> (+0.89034%) ⬆️
packages/api/src/controllers/object-store.js 56.75676% <0.00000%> (-0.77750%) ⬇️
packages/api/src/controllers/helpers.ts 48.38710% <100.00000%> (+3.05377%) ⬆️

Continue to review full report at Codecov.

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

Impacted Files Coverage Δ
packages/api/src/controllers/asset.ts 56.27010% <0.00000%> (+0.89034%) ⬆️
packages/api/src/controllers/object-store.js 56.75676% <0.00000%> (-0.77750%) ⬇️
packages/api/src/controllers/helpers.ts 48.38710% <100.00000%> (+3.05377%) ⬆️

@victorges
Copy link
Member Author

All good.

@victorges victorges merged commit 27e641d into master Aug 25, 2022
@victorges victorges deleted the vg/feat/fix-os-url-parsing branch August 25, 2022 20:34
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