fix: Add maxDownloadSize for artifact directory download #3232
Conversation
plugins/builds/artifacts/get.js
Outdated
|
|
||
| // If total size exceeds allowed limit, stop further processing | ||
| if (totalSize > maxDownloadSize) { | ||
| throw new Error('Total size of files exceeds the allowed limit of 2GB.'); |
There was a problem hiding this comment.
Limit size in error message should not be hardcoded because limit download size is configurable.
| // Check file sizes by fetching metadata | ||
| for (const file of directoryArray) { | ||
| if (file) { | ||
| const fileMetaResponse = await request.head(`${baseUrl}/${file}?token=${token}&type=download`); |
There was a problem hiding this comment.
not pressing: for large no of files fetching metadata can be time consuming, we can run in parallel with a small batch size.
There was a problem hiding this comment.
also, req.query.dir && req.query.type === 'download' , we may need schema validation for query params
There was a problem hiding this comment.
schema validation should already exist for those params
https://github.com/screwdriver-cd/screwdriver/blob/master/plugins/builds/artifacts/get.js#L171-L172
plugins/builds/artifacts/get.js
Outdated
|
|
||
| // If total size exceeds allowed limit, stop further processing | ||
| if (totalSize > maxDownloadSize) { | ||
| throw new Error(`Total size of files exceeds the allowed limit of ${maxDownloadSize}GB.`); |
There was a problem hiding this comment.
Isn't maxDownloadSize in bytes, not GB? I think it is needed to change the logging unit or convert it when outputting to the log.
Context
Would be nice if we could check for max file download size before downloading.
Objective
This PR adds config for
maxDownloadSizefor downloading artifact directories.References
Related to #3227
License
I confirm that this contribution is made under a BSD license and that I have the authority necessary to make this contribution on behalf of its copyright owner.