@actions/artifact package #304

konradpabjan · 2020-01-13T16:41:05Z

Initial commit for the @actions/artifact package. So far only contains the code for uploading an artifact, download will come in a separate PR.

Adds support for uploading artifacts with new APIs that are independent of the runner (non-plugin). The original code for uploading artifacts can be found here (see https://github.com/actions/runner/tree/master/src/Runner.Plugins/Artifact). This PR largely ports over the C# code for uploading artifacts to node and switches over to some recently added APIs. This new package will be used for the v2 versions of actions/upload-artifact and actions/download-artifact.

Been using the structure of @actions/glob in this repo as an example. For this PR I deliberately did not do tsc and npm-install so there is no lib folder or node_modules folder. Trying to keep it clean and simple for this PR so that it is easy to review.

Been testing with the following two repos for upload artifact: https://github.com/bbq-beets/artifact-package and https://github.com/bbq-beets/upload-artifact-v2

Future work not part of this PR

~~Tests (all the code in search.ts and util.ts is a good candidate, will be added to a _TEST_ directory just like in all the other packages in this repo)~~ (Tests have been added)
All the code related to downloadArtifact and downloadAllArtifacts (TODOs can be seen in src/internal-artifact.ts)
README with examples (will be added after download artifact is working and the package is almost ready to release)
lib and node_modules folder, (just like in `@actions/glob), will be added as we get close to release and everything is tested

~~Search uses our new glob package: @actions/glob~~ (searching will be externally done)
Http calls are made using our new: @actions/http-client package in order to support proxies

Much of the work that was done in @actions/cache was used as a reference in terms of how concurrent chunked uploads are done, see: actions/cache#128

Formatting was done by doing prettier --write packages src/*.ts so the style should be consistent with the other code in this repo.

packages/artifact/src/artifact.ts

packages/artifact/src/upload-artifact-http-client.ts

packages/artifact/package.json

packages/artifact/src/artifact.ts

packages/artifact/src/upload-artifact-http-client.ts

packages/artifact/src/utils.ts

packages/artifact/src/artifact.ts

packages/artifact/src/search.ts

packages/artifact/src/upload-artifact-http-client.ts

yacaovsnc · 2020-01-30T17:59:54Z

Working on some major changes (almost ready but not quite), don't review yet

Errr, didn't see this....

konradpabjan · 2020-02-03T16:13:44Z

Working on some major changes (almost ready but not quite), don't review yet

Errr, didn't see this....

Still a lot of great feedback! I should have changed the PR title so it's more obvious that it's not ready for review, I'll do that moving forward. Thanks for taking a look! 😀

packages/artifact/package.json

packages/artifact/src/config-variables.ts

yacaovsnc · 2020-02-07T18:41:44Z

packages/artifact/src/upload-http-client.ts

+
+  const parameters: UploadFileParameters[] = []
+  let continueOnError = true
+  if (options) {


This is not future proof - if we add another option in the future, this check will fail. Would
const continueOnError = !!(options && options.continueOnError)
work?

In upload-options.ts, continueOnError is a required property though, so as long as there are some uploadOptions provided this should work even if some extra properties are added to uploadOptions

If there ever are more options added to upload-options.ts, they should be required (there would be a lot of unnecessary checks if there were optional parameters in an already optional object). The general flow will be something like this?

// define default options if (options){ // update default options to whatever is specified in options }

In general, individual options in the options object should not be required. Imagine if there are many options available, user should only need to set the ones they care about.

Changed continueOnError so it is optional in upload-options.ts

Changed the rest to this:

// by default, file uploads will continue if there is an error unless specified differently in the options let continueOnError = true if (options) { if (options.continueOnError === false) { continueOnError = false } }

The double bang operators didn't seem very readable so I tried to avoid it. We do also need to support a default value if no continueOnError option is specified. Doing all that in a single line is possible but it gets pretty ugly so I think what I have is easy to follow

Also added two extra tests so now we have the following cases:

no uploadOptions

uploadOptions with continueOnError set to undefined

uploadOptions with continueOnError set to false

uploadOptions with continueOnError set to true

I think this would be more readable as Yang suggested:

const continueOnError = !!(options && options.continueOnError)

So that one liner would be fine if we didn't care about the default true behavior if no options are provided. So it just won't work

Changing to just that one line changes the expected behavior if no options are provided (2 tests start to fail).

packages/artifact/src/upload-http-client.ts

packages/artifact/src/upload-specification.ts

packages/artifact/src/upload-http-client.ts

konradpabjan · 2020-02-10T13:56:51Z

@ericsciple could you also take a 👀 at this PR. Getting ready to merge this in. A new package to the toolkit is quite a hefty addition and you're probably the best person to look at this. I have another PR pretty much ready to go after this with documentation and all the extra code related to downloading an artifact.

joshmgross

joshmgross · 2020-02-10T18:22:27Z

packages/artifact/__tests__/upload-specification.test.ts

+          path.join(artifactName, 'folder-f', 'bad-item3.txt')
+        )
+      } else {
+        throw new Error('this should never be reached')


If we're going to fail the test by throwing an error, we should be more explicit about what went wrong.

In this specific case, it might be better to simplify this into an array contains

expect(badUploadFilePaths).toContain(specification.uploadFilePath)

https://jestjs.io/docs/en/expect#tocontainitem

toContain is fairly similar to what I have with includes, I can swap that out in a few places that I use it because it seems nicer

I use an array to check for the absolutePaths however using just another array for uploadFilePath does not 100% check for what we need. It is possible that all the correct strings exist for uploadFilePath in an array, however the right combination must match with the absolutePath which is what I'm doing with the if else-if statements

joshmgross · 2020-02-10T18:25:39Z

packages/artifact/src/artifact.ts

+  }
+
+  if (uploadSpecification.length === 0) {
+    core.warning(`No files found that can be uploaded`)


Should this be a warning? It seems like an error case if there are no files to upload.

The current behavior is that and empty upload will throw a warning but not fail.

It is possible that someone includes only empty directories in the files that they want uploaded. If that is the case, the uploadspecification will be empty and this will be hit because directories get ignored by the FCS during upload.

joshmgross · 2020-02-10T18:29:45Z

packages/artifact/src/upload-http-client.ts

+
+  const parameters: UploadFileParameters[] = []
+  let continueOnError = true
+  if (options) {


I think this would be more readable as Yang suggested:

const continueOnError = !!(options && options.continueOnError)

joshmgross · 2020-02-10T18:31:17Z

packages/artifact/src/upload-http-client.ts

+  )
+
+  // eslint-disable-next-line no-console
+  console.log(`Total size of all the files uploaded is ${fileSizes} bytes`)


Minor: you can use core.info here and elsewhere if you want to console.log. It's easier to mock core than console and you don't need the eslint disables

It seems like core.info isn't documented anywhere, even though it exists 🤔 https://github.com/actions/toolkit/tree/master/packages/core

I can use it a few places, though console.log is better because it isn't limited to just strings so certain things like formatted http responses look better

…tions/toolkit into konradpabjan/actions/artifact

ericsciple · 2020-02-10T20:12:38Z

packages/artifact/src/upload-http-client.ts

+    if (isSuccessStatusCode(retryResponse.message.statusCode)) {
+      return true
+    } else {
+      // eslint-disable-next-line no-console


i think stdout is preferred so unit tests can be less noisy

e.g. here is a snippet from somewhere else in toolkit

process.stdout.write(`GITHUB_EVENT_PATH ${path} does not exist${EOL}`)

not sure, the linter may have another reason for avoiding console.log

Perhaps we could disable console.log in the jest configuration? https://stackoverflow.com/questions/44467657/jest-better-way-to-disable-console-inside-unit-tests

Some of the response logging that I have is really noisy, for the tests I'll just mock console.log so that there is no annoying output

I ended up doing the following for each of the tests

jest.spyOn(console, 'log').mockImplementation(() => {}) jest.spyOn(core, 'debug').mockImplementation(() => {}) jest.spyOn(core, 'info').mockImplementation(() => {}) jest.spyOn(core, 'warning').mockImplementation(() => {})

Shouldn't be any noise in the tests now

…tions/toolkit into konradpabjan/actions/artifact

* Initial commit for @actions/artifact package

Konrad Pabjan added 2 commits January 13, 2020 11:07

Initial commit for @actions/artifact

3a20113

Alphabetize imports

776270f

konradpabjan assigned joshmgross and yacaovsnc Jan 13, 2020

yacaovsnc reviewed Jan 13, 2020

View reviewed changes

Fix majority of lint errors

f3ac746

joshmgross reviewed Jan 14, 2020

View reviewed changes

Konrad Pabjan and others added 14 commits January 14, 2020 10:29

Fix all lint errors

bd38a3f

Various fixes due to PR feedback

b2d67bb

Add tests for search

029f2a4

Rename test file

4fda766

Test improvements

e025ef0

Misc formatting fixes

11c006c

Tests for artifactName

4431cd8

Misc improvements

7cf5770

Hopefully fix failing tests

b56cdb0

🤞 this works

6b04ca5

Hmmm

e12553e

Really weird...

83c8848

Update tests for util.ts

b6f59a0

Succesfull http mocking along with more tests

c6d23c0

yacaovsnc reviewed Jan 30, 2020

View reviewed changes

Konrad Pabjan added 6 commits January 30, 2020 13:48

Spelling fixes and misc updates

da72c73

Logic for fast failing in the event a chunk fails

206233a

Tests for artifact uploads

e6f5591

Minor fixes

7f48237

PR Feedback

ddfdb57

Format fix

0768705

konradpabjan changed the title ~~@actions/artifact package~~ @actions/artifact package - Almost ready for review Jan 31, 2020

Extra debug

cd5d31a

konradpabjan changed the title ~~@actions/artifact package - Almost ready for review~~ @actions/artifact package Feb 3, 2020

Konrad Pabjan added 6 commits February 3, 2020 20:14

Move glob search out of the package

09b1c39

Code cleanup

049110f

Rename some thing and cleanup

abd642c

Rename uploadInfo to uploadResponse

813bcf1

Misc Improvements

c8d2dd1

Format Fix

1dd4152

yacaovsnc reviewed Feb 7, 2020

View reviewed changes

Konrad Pabjan added 4 commits February 7, 2020 14:06

Test Improvements

1fa883d

Updates to continueOnError

7c6e6ac

Extra test

6b6a5e0

Normalize and Resolve all paths

4ac6ef6

yacaovsnc reviewed Feb 7, 2020

View reviewed changes

packages/artifact/src/upload-http-client.ts Outdated Show resolved Hide resolved

Change console.log to warning

d01f0f3

yacaovsnc approved these changes Feb 7, 2020

View reviewed changes

Update upload-response.ts

7917754

joshmgross approved these changes Feb 10, 2020

View reviewed changes

konradpabjan added 3 commits February 10, 2020 14:38

PR feedback

21ce5cb

Merge branch 'konradpabjan/actions/artifact' of https://github.com/ac…

78605ae

…tions/toolkit into konradpabjan/actions/artifact

Update upload-http-client.ts

fcec6a7

ericsciple reviewed Feb 10, 2020

View reviewed changes

konradpabjan added 3 commits February 10, 2020 16:21

Silence output when running tests and rename files with internal

5288533

Use Factory to create ArtifactClient

8a1220b

Merge branch 'konradpabjan/actions/artifact' of https://github.com/ac…

332dfce

…tions/toolkit into konradpabjan/actions/artifact

konradpabjan merged commit 6cbb8e9 into master Feb 11, 2020

konradpabjan deleted the konradpabjan/actions/artifact branch February 11, 2020 14:50

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

@actions/artifact package (actions#304)

c080960

* Initial commit for @actions/artifact package

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@actions/artifact package #304

@actions/artifact package #304

konradpabjan commented Jan 13, 2020 •

edited

Loading

yacaovsnc commented Jan 30, 2020

konradpabjan commented Feb 3, 2020

yacaovsnc Feb 7, 2020

konradpabjan Feb 7, 2020 •

edited

Loading

yacaovsnc Feb 7, 2020 •

edited

Loading

konradpabjan Feb 7, 2020 •

edited

Loading

joshmgross Feb 10, 2020

konradpabjan Feb 10, 2020

konradpabjan commented Feb 10, 2020

joshmgross left a comment

joshmgross Feb 10, 2020

konradpabjan Feb 10, 2020 •

edited

Loading

joshmgross Feb 10, 2020

konradpabjan Feb 10, 2020 •

edited

Loading

joshmgross Feb 10, 2020

joshmgross Feb 10, 2020

konradpabjan Feb 10, 2020

ericsciple Feb 10, 2020

ericsciple Feb 10, 2020

konradpabjan Feb 10, 2020

This comment was marked as off-topic.

konradpabjan Feb 10, 2020

@actions/artifact package #304

@actions/artifact package #304

Conversation

konradpabjan commented Jan 13, 2020 • edited Loading

yacaovsnc commented Jan 30, 2020

konradpabjan commented Feb 3, 2020

Choose a reason for hiding this comment

konradpabjan Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

yacaovsnc Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

konradpabjan Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradpabjan commented Feb 10, 2020

joshmgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradpabjan Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradpabjan Feb 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

konradpabjan commented Jan 13, 2020 •

edited

Loading

konradpabjan Feb 7, 2020 •

edited

Loading

yacaovsnc Feb 7, 2020 •

edited

Loading

konradpabjan Feb 7, 2020 •

edited

Loading

konradpabjan Feb 10, 2020 •

edited

Loading

konradpabjan Feb 10, 2020 •

edited

Loading