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

Implemented resumable downloads #26

Merged
merged 9 commits into from
Nov 8, 2022
Merged

Conversation

noxer
Copy link
Contributor

@noxer noxer commented Oct 7, 2022

Summary

This PR adds download resume functionality to mmetl.
It verifies the download by adding a small overlap with the existing file and comparing the first 512 bytes.
The HTTP client now identifies itself via the User-Agent "mmetl/1.0".

[-----existing file-----]
                [-------resumed download-------]
                [overlap]

@noxer noxer requested a review from mgdelacroix October 7, 2022 17:20
@noxer
Copy link
Contributor Author

noxer commented Oct 7, 2022

/update-branch

@noxer noxer marked this pull request as ready for review October 7, 2022 17:31
@noxer noxer requested review from agarciamontoro and removed request for mgdelacroix November 7, 2022 10:49
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

This is neat, I really enjoyed reading your code 🤓

I left some comments below, mostly nits, but also some about things I don't understand. I have a couple of general comments, too:

  • Is it possible to test the functions in download.go? They seem brittle to any future change, so I'd love to have some assertions to ensure behavior is not changed accidentally in the future.
  • Can we wrap the errors (when we just do return err) to give them a bit more context to what happened?

Thank you for your work on this, Tim! :)

services/slack/download.go Outdated Show resolved Hide resolved
services/slack/download.go Outdated Show resolved Hide resolved
services/slack/download.go Outdated Show resolved Hide resolved
services/slack/download.go Show resolved Hide resolved
services/slack/intermediate.go Outdated Show resolved Hide resolved
@agarciamontoro
Copy link
Member

agarciamontoro commented Nov 8, 2022

Oh, and also: thank you for the diagram in the PR description, it helped me understand the big picture (and now that I'm writing this I'm thinking that it would be good to add it as a comment in the download.go file explaining how things work).

@noxer
Copy link
Contributor Author

noxer commented Nov 8, 2022

I'll look into the tests. It should be possible to "fake" the request. I'll also look into the error wrapping.

@noxer
Copy link
Contributor Author

noxer commented Nov 8, 2022

I've added tests and wrapped all returned error

@noxer noxer requested a review from agarciamontoro November 8, 2022 14:48
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Cool! Great tests, thank you 🙏

services/slack/download_test.go Show resolved Hide resolved
services/slack/download_test.go Show resolved Hide resolved
@noxer noxer merged commit 8f64a09 into master Nov 8, 2022
@noxer noxer deleted the resumable-attachment-downloads branch November 8, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants