Skip to content

Comments

Cargohold: remote assets#2004

Merged
pcapriotti merged 33 commits intodevelopfrom
pcapriotti/cargohold-remote-assets
Jan 10, 2022
Merged

Cargohold: remote assets#2004
pcapriotti merged 33 commits intodevelopfrom
pcapriotti/cargohold-remote-assets

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Dec 17, 2021 •
edited by supersven
Loading

This PR implements the Cargohold federation API. There are only two endpoints:

  • get-asset, used to check if an asset exists
  • stream-asset, used to retrieve the asset contents.

The reason for having a separate get-asset RPC is that the federation API has no direct way of communicating that the asset does not exist, since non-success HTTP status codes are always interepreted as server errors by federator.

Tracked by https://wearezeta.atlassian.net/browse/FS-305.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti changed the base branch from develop to cargohold-federated-api December 20, 2021 07:31
@supersven supersven force-pushed the pcapriotti/cargohold-remote-assets branch from 8d3ff09 to 669fd74 Compare December 20, 2021 09:16
@pcapriotti pcapriotti force-pushed the cargohold-federated-api branch from 900d2b1 to a810c76 Compare December 20, 2021 09:38
@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-remote-assets branch 3 times, most recently from 558eb31 to fe43f88 Compare December 22, 2021 15:17
@supersven supersven force-pushed the pcapriotti/cargohold-remote-assets branch 3 times, most recently from ac22bdf to 3862a1c Compare December 23, 2021 18:23
@supersven supersven force-pushed the cargohold-federated-api branch from a810c76 to 31f2de4 Compare December 23, 2021 18:29
Base automatically changed from cargohold-federated-api to develop December 28, 2021 12:13
@supersven supersven force-pushed the pcapriotti/cargohold-remote-assets branch from 3862a1c to 500838e Compare December 29, 2021 16:50
@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-remote-assets branch from 2a95ce6 to e62e332 Compare January 3, 2022 15:48
@pcapriotti pcapriotti marked this pull request as ready for review January 3, 2022 15:55
@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-remote-assets branch from d955ac8 to 2ba0351 Compare January 4, 2022 12:37
mdimjasevic
mdimjasevic approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Overall, this is great!

I left some smaller comments and questions.

@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-remote-assets branch from e60c14c to 9eba8e4 Compare January 10, 2022 07:23
pcapriotti and others added 13 commits January 10, 2022 11:18
@pcapriotti
Initial implementation of remote asset download
94adf72
@pcapriotti
Add RPC to check if an asset exists
6da8a78
@pcapriotti
Download local asset for streaming
ac97cf1
@pcapriotti @supersven
Implement federation test for get-asset
864de52 
Co-authored-by: Sven Tennie <sven.tennie@wire.com>
@supersven @pcapriotti
Added get-asset test
4b73c14
@supersven @pcapriotti
Test get-asset with public and private assets
4c8d50e
@pcapriotti
WIP: federation: test large asset download
0f3a9c4
@supersven @pcapriotti
Test stream-asset
f3d62fd
@pcapriotti
Improve large asset test
994bcfd
@pcapriotti
Test streaming invalid assets
ac8ecb9
@supersven @pcapriotti
Introduce TestM in Cargohold integration tests
3916483 
This ReaderT gives access to the HTTP manager and the Cargohold
endpoint. The wrapped monad is HTTP (to make requests).
@pcapriotti
Reintroduce tasty resource for configuration file
74950bb 
This avoids having to read the configuration file before every test.
@pcapriotti
Add more API client utilities in tests
f42e4ca
pcapriotti and others added 20 commits January 10, 2022 11:18
@pcapriotti
Use servant client for federation tests
27a7f26
@pcapriotti
Set up federator mocking in cargohold
6511732
@supersven @pcapriotti
Fix formatting
09c2ddb
@supersven @pcapriotti
Assert the content of a remote asset download
1ca2d49
@supersven @pcapriotti
Add changelog.d entry
6c4bcf2
@pcapriotti
Test 404 when fetching remote asset
3e46eec
@pcapriotti
Check federated requests in remote asset test
c090908
@pcapriotti
Add config to cargohold integration chart
4042598
@pcapriotti
Simple end2end remote asset download test
f1717d9
@pcapriotti
Remove unused uploadRandom function
cb7261a
@pcapriotti
fixup! Simple end2end remote asset download test
29ce923
@pcapriotti
Enable federation in cargohold helm chart
99857ff
@pcapriotti
Make asset public in end-to-end test
6d4b224
@pcapriotti
Set hostname for instance with mocked federator
fde0588
@pcapriotti
fixup! Set hostname for instance with mocked federator
a21fff1
@pcapriotti
Fix local domain in fed request assertions
a9347f6
@pcapriotti
Fix cargohold test in federator
22846e8
@pcapriotti
Use isJust
2a601de 
Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
@pcapriotti
Simplify implementation of getAsset
10bf79e 
Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
@pcapriotti
Remove redundant $
17818c1 
Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-remote-assets branch from 9eba8e4 to 17818c1 Compare January 10, 2022 10:23
@pcapriotti pcapriotti merged commit fd198b7 into develop Jan 10, 2022
@pcapriotti pcapriotti deleted the pcapriotti/cargohold-remote-assets branch January 10, 2022 12:38
@mdimjasevic mdimjasevic mentioned this pull request Jan 12, 2022
Merged
4 tasks
@smatting smatting mentioned this pull request Jan 12, 2022
Merged
2 tasks
@akshaymankar akshaymankar mentioned this pull request Jan 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mdimjasevic mdimjasevic mdimjasevic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pcapriotti @mdimjasevic @supersven