Initial commit to create @actions/cache package #448

aiqiaoy · 2020-05-06T15:39:04Z

Initial commit to create the @actions/cache package. ADR is here.

The original code can be found under the cache repo here. Differences between cache actions and the node package are:

Keys, paths and concurrency settings are input and environment variables in cache action. Here they are function parameters.
For save and restore failures, cache action log warnings while the node package throw errors and allow consumers to decide what to do.

Last commit included in this PR is: bac1a40

Addresses:
https://github.com/github/c2c-actions-service/issues/533
actions/cache#55

packages/cache/src/internal/cacheHttpClient.ts

joshmgross

Doesn't have to be in this PR, but we should consider adding some E2E workflow tests for this package, similar to https://github.com/actions/toolkit/blob/master/.github/workflows/artifact-tests.yml and what we have already for https://github.com/actions/cache

README.md

packages/cache/RELEASES.md

packages/cache/package.json

packages/cache/src/cache.ts

packages/cache/src/internal/cacheHttpClient.ts

README.md

.github/workflows/cache-tests.yml

aiqiaoy · 2020-05-12T18:57:48Z

@joshmgross @dhadka @thboop Finally got all tests to pass. PR is ready for review 😄

konradpabjan

Overall looks really really good 👍 A few minor things

packages/cache/src/internal/constants.ts

README.md

packages/cache/README.md

packages/cache/__tests__/restoreCache.test.ts

packages/cache/src/internal/cacheUtils.ts

konradpabjan · 2020-05-13T19:46:17Z

packages/cache/src/internal/cacheHttpClient.ts

+      )
+    }
+  } else {
+    core.debug('Unable to validate download, no Content-Length header')


Just curious, what are the implications of not properly validating the download? If this is serious then maybe core.warning should be used?

konradpabjan · 2020-05-13T19:48:49Z

packages/cache/src/internal/cacheHttpClient.ts

+  const response = await retryTypedResponse('getCacheEntry', async () =>
+    httpClient.getJson<ArtifactCacheEntry>(getCacheApiUrl(resource))
+  )
+  if (response.statusCode === 204) {


Not needed for this PR, but 204 could be added to the HTTP Codes in our http-client

konradpabjan · 2020-05-13T19:50:50Z

packages/cache/src/cache.ts

+import {createTar, extractTar} from './internal/tar'
+import {UploadOptions} from './options'
+
+export class ValidationError extends Error {


This is a pretty cool pattern! 👍

konradpabjan · 2020-05-13T19:54:51Z

packages/cache/src/cache.ts

+
+  const compressionMethod = await utils.getCompressionMethod()
+
+  core.debug('Reserving Cache')


Just an observation, core.debug is used pretty frequently, but I think there should be some more uses of core.info so users see more logs and information.

Most users won't have step-debugging turned on and I've noticed that a lot of our first party actions don't have the greatest output/logging.

Package users/Action Authors might not want that verbose logging from a package they consume. I'd say leave as-is and if we get feedback we can add an option to the package to add verbose logging rather than verbose by default

Fine for now, just be aware that once actions/cache switches over to using this package, the same level of logging will apply (unless there is an option)

konradpabjan · 2020-05-13T20:12:37Z

Side note: I really like documentation and visuals 🙂 For the artifacts package I added some charts to help make it easier for new users to understand what is going on (and maintainers when they need a refresh in the future). You can see it here: https://github.com/actions/toolkit/blob/master/packages/artifact/docs/implementation-details.md#uploadcompression-flow

Absolutely not needed for this PR, but I think it would be pretty nice to have something similar for this package. Even some basic charts about how the decision is made between zstd and gzip or how the retry logic is handled. Even a few things should make it easier for open-source contributors to understand things quicker.

joshmgross

Looks good to me!

packages/cache/src/cache.ts

joshmgross · 2020-05-13T20:28:54Z

packages/cache/src/internal/cacheUtils.ts

+export async function createTempDirectory(): Promise<string> {
+  const IS_WINDOWS = process.platform === 'win32'
+
+  let tempDirectory: string = process.env['RUNNER_TEMP'] || ''


See below, if tempDIrectory is empty we create our own temp directory. Note this is an edge case, as RUNNER_TEMP should always be defined.

packages/cache/src/internal/cacheUtils.ts

packages/cache/src/options.ts

joshmgross · 2020-05-13T20:35:24Z

packages/cache/src/cache.ts

+
+  const compressionMethod = await utils.getCompressionMethod()
+
+  core.debug('Reserving Cache')


Package users/Action Authors might not want that verbose logging from a package they consume. I'd say leave as-is and if we get feedback we can add an option to the package to add verbose logging rather than verbose by default

joshmgross · 2020-05-13T20:37:45Z

packages/cache/README.md

+const cacheId = await cache.saveCache(paths, key)
+```
+
+## Additional Documentation


Once actions/cache starts to consume this package, we can update this doc to point to it as an example implementation.

thboop

Some minor thoughts but this PR looks great 🥇

README.md

packages/cache/README.md

thboop · 2020-05-13T18:53:58Z

packages/cache/RELEASES.md

@@ -0,0 +1,5 @@
+# @actions/cache Releases
+
+### 1.0.0


Should we consider doing a beta release and letting users try it out and provide feedback before we launch 1.0.0?

We can create an issue inviting users to try it out, and iterate on feedback they provide!

Any guideline on how to do beta release? Set the release to 0.0.0?

With artifacts, we had a 0.1.0 release: https://github.com/actions/toolkit/blob/master/packages/artifact/RELEASES.md

The other toolkit packages used 1.0.0 as the initial release, I'd prefer if we could do that, but it doesn't make a big difference either way.

Whether or not we want to do a beta release is another discussion.

If you set the version to 1.0.0-beta.0 we can push a beta tag using npm publish --tag beta. Just make sure whoever is doing the first publish (I can) knows.

Since we are re-using a lot of this code and feel confident about the behavior and interfaces, we can also opt to skip a beta release and just release the package.

scripts/create-cache-files.sh

packages/cache/src/internal/cacheUtils.ts

packages/cache/src/internal/tar.ts

.github/workflows/cache-tests.yml

packages/cache/README.md

thboop

LGTM!

konradpabjan

🎉🎈 Ship it! 🚀

Initial commit to create @actions/cache package

aiqiaoy requested review from bryanmacfarlane, dhadka and joshmgross May 6, 2020 15:39

aiqiaoy force-pushed the users/aiyan/cache-package branch from 7bdca7e to 509f795 Compare May 6, 2020 18:51

aiqiaoy commented May 6, 2020

View reviewed changes

packages/cache/src/internal/cacheHttpClient.ts Outdated Show resolved Hide resolved

joshmgross reviewed May 6, 2020

View reviewed changes

aiqiaoy force-pushed the users/aiyan/cache-package branch from 509f795 to f5ca1ba Compare May 6, 2020 22:01

aiqiaoy marked this pull request as draft May 6, 2020 22:11

aiqiaoy force-pushed the users/aiyan/cache-package branch from b5fa458 to 25662a5 Compare May 7, 2020 20:16

aiqiaoy marked this pull request as ready for review May 7, 2020 20:26

aiqiaoy commented May 8, 2020

View reviewed changes

packages/cache/src/internal/cacheHttpClient.ts Outdated Show resolved Hide resolved

thboop reviewed May 8, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

konradpabjan reviewed May 11, 2020

View reviewed changes

.github/workflows/cache-tests.yml Outdated Show resolved Hide resolved

Aiqiao Yan and others added 5 commits May 12, 2020 12:02

Initial commit to create @actions/cache package

932779c

Change variable path to a list

7409ad5

Fix tests

15fefd9

Add docs and tests

c534ad2

Add cache upload options and pull from latest actions/cache master

1413cd0

aiqiaoy force-pushed the users/aiyan/cache-package branch from 25662a5 to 1413cd0 Compare May 12, 2020 16:54

Attempt to fix the test

b3c8e19

aiqiaoy force-pushed the users/aiyan/cache-package branch from 11f82b2 to b3c8e19 Compare May 13, 2020 17:43

aiqiaoy requested a review from ericsciple May 13, 2020 17:52

konradpabjan reviewed May 13, 2020

View reviewed changes

joshmgross approved these changes May 13, 2020

View reviewed changes

thboop reviewed May 13, 2020

View reviewed changes

konradpabjan mentioned this pull request May 14, 2020

Remove --save in README #455

Merged

thboop approved these changes May 15, 2020

View reviewed changes

konradpabjan approved these changes May 15, 2020

View reviewed changes

React to feedback

d2b2399

aiqiaoy force-pushed the users/aiyan/cache-package branch from 4dcea16 to d2b2399 Compare May 15, 2020 16:26

aiqiaoy merged commit a67b91e into master May 15, 2020

aiqiaoy mentioned this pull request May 15, 2020

Can this module be used from other JavaScript actions? actions/cache#55

Closed

aiqiaoy deleted the users/aiyan/cache-package branch May 20, 2020 14:44

at-wat pushed a commit to at-wat/actions-toolkit that referenced this pull request Aug 31, 2023

Merge pull request actions#448 from actions/users/aiyan/cache-package

623465d

Initial commit to create @actions/cache package

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit to create @actions/cache package #448

Initial commit to create @actions/cache package #448

aiqiaoy commented May 6, 2020 •

edited

joshmgross left a comment

aiqiaoy commented May 12, 2020

konradpabjan left a comment

konradpabjan May 13, 2020

konradpabjan May 13, 2020

konradpabjan May 13, 2020

konradpabjan May 13, 2020 •

edited

joshmgross May 13, 2020

konradpabjan May 15, 2020

konradpabjan commented May 13, 2020

joshmgross left a comment

joshmgross May 13, 2020

joshmgross May 13, 2020

joshmgross May 13, 2020

thboop left a comment

thboop May 13, 2020

aiqiaoy May 15, 2020

konradpabjan May 15, 2020

thboop May 15, 2020 •

edited

thboop left a comment

konradpabjan left a comment


		const compressionMethod = await utils.getCompressionMethod()

		core.debug('Reserving Cache')

Initial commit to create @actions/cache package #448

Initial commit to create @actions/cache package #448

Conversation

aiqiaoy commented May 6, 2020 • edited

joshmgross left a comment

Choose a reason for hiding this comment

aiqiaoy commented May 12, 2020

konradpabjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradpabjan May 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradpabjan commented May 13, 2020

joshmgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thboop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thboop May 15, 2020 • edited

Choose a reason for hiding this comment

thboop left a comment

Choose a reason for hiding this comment

konradpabjan left a comment

Choose a reason for hiding this comment

aiqiaoy commented May 6, 2020 •

edited

konradpabjan May 13, 2020 •

edited

thboop May 15, 2020 •

edited