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

Include fs stats and mode in zip archive, fix "too many open files" error #1723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 30 additions & 15 deletions packages/artifact/__tests__/upload-artifact.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,18 @@ describe('upload-artifact', () => {
.mockReturnValue([
{
sourcePath: '/home/user/files/plz-upload/file1.txt',
destinationPath: 'file1.txt'
destinationPath: 'file1.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/file2.txt',
destinationPath: 'file2.txt'
destinationPath: 'file2.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/dir/file3.txt',
destinationPath: 'dir/file3.txt'
destinationPath: 'dir/file3.txt',
stats: new fs.Stats()
}
])

Expand Down Expand Up @@ -139,15 +142,18 @@ describe('upload-artifact', () => {
.mockReturnValue([
{
sourcePath: '/home/user/files/plz-upload/file1.txt',
destinationPath: 'file1.txt'
destinationPath: 'file1.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/file2.txt',
destinationPath: 'file2.txt'
destinationPath: 'file2.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/dir/file3.txt',
destinationPath: 'dir/file3.txt'
destinationPath: 'dir/file3.txt',
stats: new fs.Stats()
}
])

Expand Down Expand Up @@ -178,15 +184,18 @@ describe('upload-artifact', () => {
.mockReturnValue([
{
sourcePath: '/home/user/files/plz-upload/file1.txt',
destinationPath: 'file1.txt'
destinationPath: 'file1.txt',
stats: fs.Stats.prototype
},
{
sourcePath: '/home/user/files/plz-upload/file2.txt',
destinationPath: 'file2.txt'
destinationPath: 'file2.txt',
stats: fs.Stats.prototype
},
{
sourcePath: '/home/user/files/plz-upload/dir/file3.txt',
destinationPath: 'dir/file3.txt'
destinationPath: 'dir/file3.txt',
stats: fs.Stats.prototype
}
])

Expand Down Expand Up @@ -233,15 +242,18 @@ describe('upload-artifact', () => {
.mockReturnValue([
{
sourcePath: '/home/user/files/plz-upload/file1.txt',
destinationPath: 'file1.txt'
destinationPath: 'file1.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/file2.txt',
destinationPath: 'file2.txt'
destinationPath: 'file2.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/dir/file3.txt',
destinationPath: 'dir/file3.txt'
destinationPath: 'dir/file3.txt',
stats: new fs.Stats()
}
])

Expand Down Expand Up @@ -296,15 +308,18 @@ describe('upload-artifact', () => {
.mockReturnValue([
{
sourcePath: '/home/user/files/plz-upload/file1.txt',
destinationPath: 'file1.txt'
destinationPath: 'file1.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/file2.txt',
destinationPath: 'file2.txt'
destinationPath: 'file2.txt',
stats: new fs.Stats()
},
{
sourcePath: '/home/user/files/plz-upload/dir/file3.txt',
destinationPath: 'dir/file3.txt'
destinationPath: 'dir/file3.txt',
stats: new fs.Stats()
}
])

Expand Down
54 changes: 25 additions & 29 deletions packages/artifact/src/internal/upload/upload-zip-specification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,19 @@ import {validateFilePath} from './path-and-artifact-name-validation'

export interface UploadZipSpecification {
/**
* An absolute source path that points to a file that will be added to a zip. Null if creating a new directory
* An absolute source path that points to a file or directory that will be added to a zip
*/
sourcePath: string | null
sourcePath: string

/**
* The destination path in a zip for a file
*/
destinationPath: string

/**
* Metadata (permissions, creation time, etc) about the file
*/
stats: fs.Stats
}

/**
Expand Down Expand Up @@ -75,37 +80,28 @@ export function getUploadZipSpecification(
- file3.txt
*/
for (let file of filesToZip) {
if (!fs.existsSync(file)) {
const stats = fs.statSync(file, {throwIfNoEntry: false});
if (!stats) {
throw new Error(`File ${file} does not exist`)
}
if (!fs.statSync(file).isDirectory()) {
// Normalize and resolve, this allows for either absolute or relative paths to be used
file = normalize(file)
file = resolve(file)
if (!file.startsWith(rootDirectory)) {
throw new Error(
`The rootDirectory: ${rootDirectory} is not a parent directory of the file: ${file}`
)
}

// Check for forbidden characters in file paths that may cause ambiguous behavior if downloaded on different file systems
const uploadPath = file.replace(rootDirectory, '')
validateFilePath(uploadPath)
// Normalize and resolve, this allows for either absolute or relative paths to be used
file = normalize(file)
file = resolve(file)
if (!file.startsWith(rootDirectory)) {
throw new Error(
`The rootDirectory: ${rootDirectory} is not a parent directory of the file: ${file}`
)
}

specification.push({
sourcePath: file,
destinationPath: uploadPath
})
} else {
// Empty directory
const directoryPath = file.replace(rootDirectory, '')
validateFilePath(directoryPath)
// Check for forbidden characters in file paths that may cause ambiguous behavior if downloaded on different file systems
const destinationPath = file.replace(rootDirectory, '')
validateFilePath(destinationPath)

specification.push({
sourcePath: null,
destinationPath: directoryPath
})
}
specification.push({
sourcePath: file,
destinationPath,
stats
})
}
return specification
}
14 changes: 10 additions & 4 deletions packages/artifact/src/internal/upload/zip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,20 @@ export async function createZipUploadStream(
zip.on('end', zipEndCallback)

for (const file of uploadSpecification) {
if (file.sourcePath !== null) {
if (!file.stats.isDirectory()) {
// Add a normal file to the zip
zip.append(createReadStream(file.sourcePath), {
name: file.destinationPath
zip.file(file.sourcePath, {
name: file.destinationPath,
stats: file.stats,
mode: file.stats.mode
})
} else {
// Add a directory to the zip
zip.append('', {name: file.destinationPath})
zip.append('', {
name: file.destinationPath,
stats: file.stats,
mode: file.stats.mode
})
}
}

Expand Down