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

@actions/artifact package updates #408

Merged
merged 4 commits into from
Apr 9, 2020
Merged

Conversation

konradpabjan
Copy link
Contributor

Overview

  • Prepare for 0.3.0 release
    • Update RELEASES.md with changelog info
    • Bump 0.2.0 -> 0.3.0 in package.json and package-lock.json
  • Storage Quota Error
    • During artifact upload, if a user has hit their storage quota, a 403 might be returned when trying to create a new container for an artifact. There is now a clear error message that lets the user know they can't upload any more artifacts because of storage. For more info, see Try out v2-preview upload-artifact#62 (comment)
    • Extra test has been added to upload.test.ts to test a 403 response code along with an extra test in util.test.ts
  • Empty file downloads
    • The v1 action of download-artifact has some extra logic when dealing with empty files. You can find it here. The updates in the PR largely aim to replicate this behavior so there are no extra HTTP calls made when downloading empty files.
    • download-specification has been updated with a new emptyFilesToCreate array that keeps track of empty files that should be created
    • util.ts has been updated with a new method that creates empty files
    • tests have been added to make sure the download-specification correctly picks up empty files and that empty files actually get created
  • Misc
    • Some asyncification of some previous sync methods that were used in tests
    • Small spelling error fixes

Testing

@@ -296,3 +304,11 @@ export async function createDirectoriesForArtifact(
})
}
}

export async function createEmptyFilesForArtifact(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the guidance here on how to create an empty file in node.

// ensure they are first created by using the createDirectoriesForArtifact method
const directoryStructure = [root, directoryToCreate]
await utils.createDirectoriesForArtifact(directoryStructure)
await expect(fs.promises.access(root)).resolves.toEqual(undefined)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For asynchronously checking if files exist, I used this for guidance

Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with fs.existsSync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fs.existsSync is perfectly fine however I'm trying to use async/promises wherever possible. In general any sync operation will block the event loop until the operation is complete so asyncification is usually a good thing.

Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple of comments but they're not blocking

packages/artifact/__tests__/download-specification.test.ts Outdated Show resolved Hide resolved
// ensure they are first created by using the createDirectoriesForArtifact method
const directoryStructure = [root, directoryToCreate]
await utils.createDirectoriesForArtifact(directoryStructure)
await expect(fs.promises.access(root)).resolves.toEqual(undefined)
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with fs.existsSync?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants