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/vod: Inline storage info in the asset resource #1014

Merged
merged 24 commits into from
May 26, 2022

Conversation

victorges
Copy link
Member

@victorges victorges commented Apr 20, 2022

What does this pull request do? Explain your changes. (required)

This is an improvement proposal to the VOD API, from some feedback that we've already got from users
and evolutions we would like to make to the API for easier use, like. for example for abstraction-matching
the new VOD dashboard pages. The changes are specifically regarding the relation between assets and
tasks and more extensively with the export to IPFS task.

The idea is that instead of having an asset loosely tied to a bunch of export tasks, it will now have explicit
fields for the current desired storage for the asset and status for those storages (as it can take a while until
the desired state becomes reality). This storage is currently only IPFS, but we will add other d3 storages in
the future. The object store ID is not changeable for now.

There's a whole migration strategy for the status field of the assets that have changed:

  • for now, would deploy this change with backward compatibility code implemented directly in the DB layer
  • created a migration API which, once we're confident with the change, should be called until all assets are migrated
  • after that (migration API starts returning 0), we can remove the compatibility code

We are actually breaking the public schema of assets, but I believe this should be OK since it's not being
used in many places. This new design is more extensible and will allow us to make other changes in the future
without breaking compatibility again.

Specific updates (required)

  • Make the status field in the asset be an object just like from tasks. The previous status becomes status.phase and the previous updatedAt becomes status.updatedAt.
  • Create a storage field in the asset and allow patching it
  • Update storage and status field on the right places (when exporting an asset on IPFS, when tasks finish, etc)
  • Update www code to use the new status field (all responses will come with that from day1, old schema will be only in the DB)
  • Update VOD documentation with new asset schema

Pending:

  • TODO after deploying this: Update video-nft SDK with new asset schema

-

  • How did you test each of these updates (required)
    yarn test, running all the new VOD tests!

I'll try to add some unit tests here while this gets reviewed, otherwise will test locally and/or on staging.

Does this pull request close any open issues?

Implements #1020

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.

@vercel
Copy link

vercel bot commented Apr 20, 2022

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

Name Status Preview Updated
livepeer-com ✅ Ready (Inspect) Visit Preview May 25, 2022 at 9:33PM (UTC)

@victorges victorges force-pushed the vg/feat/asset-ipfs-builtin branch from 489bab1 to 406227f Compare May 16, 2022 22:10
@victorges victorges force-pushed the vg/feat/asset-ipfs-builtin branch from 1ebf61d to d63aa24 Compare May 16, 2022 23:01
@victorges victorges changed the title Vg/feat/asset ipfs builtin api/vod: Inline storage information in the asset resource May 17, 2022
@victorges victorges changed the title api/vod: Inline storage information in the asset resource api/vod: Inline storage info in the asset resource May 17, 2022
@victorges victorges marked this pull request as ready for review May 17, 2022 18:35
@victorges victorges requested a review from a team as a code owner May 17, 2022 18:35
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.

Saving my comments mid review before I lose them

packages/api/src/controllers/asset.ts Show resolved Hide resolved
packages/api/src/controllers/asset.ts Show resolved Hide resolved
packages/api/src/schema/schema.yaml Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #1014 (6d12ce8) into master (42ed0b5) will increase coverage by 2.49822%.
The diff coverage is 58.94737%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1014         +/-   ##
===================================================
+ Coverage   46.14048%   48.63870%   +2.49822%     
===================================================
  Files             64          65          +1     
  Lines           4029        4077         +48     
  Branches         679         689         +10     
===================================================
+ Hits            1859        1983        +124     
+ Misses          1971        1866        -105     
- Partials         199         228         +29     
Impacted Files Coverage Δ
packages/api/src/app-router.ts 54.05405% <ø> (ø)
packages/api/src/webhooks/cannon.ts 48.25581% <0.00000%> (-0.28221%) ⬇️
packages/api/src/task/scheduler.ts 41.53846% <28.57143%> (+35.98290%) ⬆️
packages/api/src/controllers/asset.ts 36.68122% <65.38462%> (+29.36415%) ⬆️
packages/api/src/index.ts 49.09091% <100.00000%> (ø)
packages/api/src/store/asset-table.ts 100.00000% <100.00000%> (ø)
packages/api/src/store/db.ts 82.02247% <100.00000%> (ø)
packages/api/src/test-server.ts 93.02326% <100.00000%> (+0.16611%) ⬆️
packages/api/src/store/table.ts 68.27586% <0.00000%> (+1.37930%) ⬆️
packages/api/src/controllers/task.ts 13.26531% <0.00000%> (+4.08164%) ⬆️
... and 3 more

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 42ed0b5...6d12ce8. Read the comment docs.

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

@victorges victorges merged commit c691933 into master May 26, 2022
@victorges victorges deleted the vg/feat/asset-ipfs-builtin branch May 26, 2022 23:28
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.

2 participants