Artifact upload: support uploading single un-zipped files #2256
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for uploading single files without archiving them into a zip file. The feature introduces a new skipArchive option to UploadArtifactOptions that, when enabled, uploads a single file directly with its original MIME type instead of creating a zip archive.
Changes:
- Added
skipArchiveoption to allow uploading single uncompressed files - Introduced MIME type detection and transmission to the artifact service
- Refactored stream handling code into a shared module for reusability
- Updated artifact API version from 4 to 7 to support the new MIME type field
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/artifact/src/internal/upload/types.ts | New file providing MIME type mapping for various file extensions |
| packages/artifact/src/internal/upload/stream.ts | New file containing extracted and new stream utilities, including WaterMarkedUploadStream class and createRawFileUploadStream function |
| packages/artifact/src/internal/upload/zip.ts | Refactored to use WaterMarkedUploadStream from the new stream module |
| packages/artifact/src/internal/upload/blob-upload.ts | Renamed uploadZipToBlobStorage to uploadToBlobStorage and added contentType parameter |
| packages/artifact/src/internal/upload/upload-artifact.ts | Main logic for skipArchive feature including validation, file handling, and artifact name adjustment |
| packages/artifact/src/internal/shared/interfaces.ts | Added skipArchive option to UploadArtifactOptions interface with documentation |
| packages/artifact/src/generated/results/api/v1/artifact.ts | Generated protobuf code with mimeType field added to CreateArtifactRequest, removed migration-related types, and updated RepeatType |
| packages/artifact/src/generated/results/api/v1/artifact.twirp-client.ts | Minor formatting change (trailing newline) |
| packages/artifact/tests/upload-artifact.test.ts | Comprehensive test coverage for skipArchive feature including multiple file validation, raw file upload, MIME type handling, and artifact naming |
Comments suppressed due to low confidence (1)
packages/artifact/src/internal/upload/upload-artifact.ts:35
- The validation for skipArchive doesn't check if the files array is empty. If files.length is 0, the error won't be thrown, but then files[0] will be undefined on line 41, leading to unexpected behavior. Add a check for files.length === 0 alongside the files.length > 1 check.
if (files.length > 1){
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should throw an error if skipArchive is true and multiple files are provided', async () => { | ||
| const uploadResp = uploadArtifact( | ||
| fixtures.inputs.artifactName, | ||
| fixtures.inputs.files, | ||
| fixtures.inputs.rootDirectory, | ||
| {skipArchive: true} | ||
| ) | ||
|
|
||
| await expect(uploadResp).rejects.toThrow( | ||
| 'skipArchive option is only supported when uploading a single file' | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Missing test coverage for the edge case where skipArchive is true but the files array is empty (files.length === 0). This scenario would lead to accessing files[0] which is undefined, causing unexpected behavior. Consider adding a test case for this scenario.
| if (!options?.skipArchive && zipSpecification.length === 0) { | ||
| throw new FilesNotFoundError( | ||
| zipSpecification.flatMap(s => (s.sourcePath ? [s.sourcePath] : [])) | ||
| ) | ||
| } |
There was a problem hiding this comment.
When skipArchive is true and zipSpecification.length is 0 (meaning the file doesn't exist or can't be found), the FilesNotFoundError won't be thrown because of the condition on line 53. This means that a non-existent file could be passed to createRawFileUploadStream on line 93, which would fail later in the process. Consider checking if the file exists when skipArchive is true, or restructure the condition to also throw FilesNotFoundError when skipArchive is true and zipSpecification is empty.
There was a problem hiding this comment.
2026-01-27T14:53:58.8928253Z Current runner version: '2.331.0'
| ): Promise<UploadArtifactResponse> { | ||
| let artifactFileName = `${name}.zip` | ||
| if (options?.skipArchive) { | ||
| if (files.length > 1){ |
There was a problem hiding this comment.
Missing space after the closing parenthesis and before the opening brace. This is inconsistent with standard JavaScript/TypeScript formatting conventions.
| if (files.length > 1){ | |
| if (files.length > 1) { |
| } | ||
|
|
||
| artifactFileName = path.basename(files[0]) | ||
| name = artifactFileName |
There was a problem hiding this comment.
Reassigning the 'name' parameter can be confusing and error-prone. When skipArchive is true, the original artifact name provided by the user is discarded and replaced with the filename. This could be unexpected behavior. Consider using a separate variable like 'actualArtifactName' to hold the final name value while keeping the original 'name' parameter unchanged, making the code flow clearer.
| fileStream.on('error', error => { | ||
| core.error('An error has occurred while reading the file for upload') | ||
| core.info(String(error)) | ||
| throw new Error('An error has occurred during file read for the artifact') |
There was a problem hiding this comment.
Throwing an error inside an event handler won't propagate properly in an async context. The error will be thrown in the event handler's context, not in the async function's context, which means it won't be caught by normal try-catch blocks or Promise rejection handlers. Consider destroying the stream with the error instead using uploadStream.destroy(error) or emitting an 'error' event on the uploadStream.
| throw new Error('An error has occurred during file read for the artifact') | |
| uploadStream.destroy( | |
| error instanceof Error | |
| ? error | |
| : new Error('An error has occurred during file read for the artifact') | |
| ) |
|
Thanks for this! The ability to upload single un-zipped files with the appropriate MIME type will unlock real workflows where ZIP packaging is actively harmful. I previously opened #1874, which aimed at improving download behavior when artifacts are not zipped. That PR was ultimately closed, but it surfaced a real pain point: today the toolkit and While here it focuses on the upload side, it aligns with that same broader goal: support first-class non-zip artifacts. Together, these capabilities would cover both ends of the artifact lifecycle for non-zipped files and improve compatibility with existing external producers/consumers of artifacts. For example, workflows using gzip blobs (e.g., |
Description
Sometimes we want to upload un-zipped files:
tarfiles to handle file permissions. Zipping those files is unhelpful since it requires double decompressing.pdf,png/jpg,json,html, etc.). To support something like that in the UI, we can't have them zippedThis PR adds support for those scenarios.
Changes:
Notes
skipArchiveistrue, thenameparameter is replaced with the file name.skipArchiveisfalse. Notably, this'll mean the file type is not present in the artifact name. The mime type passed will still beapplication/zip, though.