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

Added support for chunked uploads. #1208

Merged

Conversation

grigorye
Copy link
Contributor

@grigorye grigorye commented Jun 10, 2022

This is follow-up for #1206, eventually addressing #677 (comment).

It implements a naive handler that just appends every chunk, assuming that upload-artifact does the chunk upload sequentially.

It passes the tests introduced for chunked uploads, that were failing in #1206, so I wonder if it might be enough for starters.

@grigorye grigorye force-pushed the feature/Chunked-Uploads-With-Multiple-Requests branch from 44bc553 to 6821bfa Compare June 10, 2022 11:16
@grigorye grigorye marked this pull request as ready for review June 10, 2022 11:41
@grigorye grigorye requested a review from a team as a code owner June 10, 2022 11:41
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #1208 (bb420d1) into master (4f8da0a) will increase coverage by 4.81%.
The diff coverage is 76.32%.

@@            Coverage Diff             @@
##           master    #1208      +/-   ##
==========================================
+ Coverage   57.50%   62.32%   +4.81%     
==========================================
  Files          32       40       +8     
  Lines        4594     5417     +823     
==========================================
+ Hits         2642     3376     +734     
- Misses       1729     1776      +47     
- Partials      223      265      +42     
Impacted Files Coverage Δ
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/model/workflow.go 54.29% <0.00%> (+3.38%) ⬆️
pkg/container/docker_run.go 15.18% <21.73%> (+9.64%) ⬆️
pkg/container/docker_pull.go 33.33% <33.33%> (ø)
pkg/container/file_collector.go 44.85% <44.85%> (ø)
pkg/common/git/git.go 50.00% <47.91%> (ø)
pkg/container/docker_auth.go 47.61% <50.00%> (+2.61%) ⬆️
pkg/exprparser/interpreter.go 73.37% <53.48%> (-0.02%) ⬇️
pkg/model/planner.go 50.73% <60.00%> (+0.32%) ⬆️
... and 30 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ChristopherHX
ChristopherHX previously approved these changes Jun 10, 2022
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this, I actually don't using the artifact feature myself.
I trust your tests with uploading / downloading large random data, far better than before.

@grigorye grigorye force-pushed the feature/Chunked-Uploads-With-Multiple-Requests branch from bfc42a0 to b6ae8b2 Compare June 16, 2022 09:55
@grigorye
Copy link
Contributor Author

@ChristopherHX may you please take a look at this extra commit, that should fix the problem I introduced earlier (appending to instead of overwriting stale artifacts): b6ae8b2

@mergify mergify bot requested a review from a team June 16, 2022 12:44
@ChristopherHX
Copy link
Contributor

ChristopherHX commented Jun 16, 2022

Please add a test for "appending to instead of overwriting stale artifacts". E.g uploading a file with different sizes to the same artifact and path. I read again...

BTW I use this dirty code to parse the actual values of the range request: https://github.com/ChristopherHX/runner.server/blob/main/src/Runner.Server/Controllers/ArtifactController.cs#L150-L155

@grigorye
Copy link
Contributor Author

grigorye commented Jun 16, 2022

BTW I use this dirty code to parse the actual values of the range request: https://github.com/ChristopherHX/runner.server/blob/main/src/Runner.Server/Controllers/ArtifactController.cs#L150-L155

Yep, thanks! I also thought about further improving the parsing of Content-Range, however, given that my Go knowledge is basically non-existent, I decided to keep the parsing to the bare minimum that is probably enough "for now"...

Copy link
Member

@KnisterPeter KnisterPeter left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this

@cplee cplee merged commit 7105919 into nektos:master Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants