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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi Path Artifact Upload + Exclude Character Support #94

Merged
merged 8 commits into from Jul 9, 2020

Conversation

konradpabjan
Copy link
Collaborator

@konradpabjan konradpabjan commented Jul 9, 2020

Resolves #44 and #55

Overview

The @actions/glob npm package is used internally to search for files to upload. This package supports exclude characters, but it is useless if you can only provide a single path at a time as input. This PR adds support for correctly uploading artifacts that have multiple paths provided as input. Previously an exception would be thrown if multiple search paths were detected. The glob package accepts multiple search paths if they are separated by newline characters so using path alongside | makes it very easy.

The glob package has a getSearchPaths function that returns the search paths that were used to find all the files. If multiple paths are used, there is the possibility that there are more than 1 search paths returned by this function.

If more than 1 search path is returned, the least common ancestor of all paths is calculated and that will be used as the root directory of the artifact. The README has been updated with information to describe this behavior along with an example. I've also added an example for the "flattening" that currently happens if wildcard patterns are used.

馃帹 New Readme 馃枌

Testing

There are no compat problems or breaking changes with this addition. Anyone using the v2 action with either an individual file, directory or wildcard pattern will not be effected and all existing behavior stays the same.

Regarding the new behavior:

  • Extra unit tests added using jest
  • E2E tests added that run for each PR
  • Local testing with earlier fork

@konradpabjan konradpabjan marked this pull request as ready for review July 9, 2020 10:49
@konradpabjan konradpabjan marked this pull request as draft July 9, 2020 11:26
@konradpabjan konradpabjan marked this pull request as ready for review July 9, 2020 11:36
README.md Outdated Show resolved Hide resolved
src/search.ts Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@gimenete gimenete left a comment

Choose a reason for hiding this comment

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

Looks good!

Just minor suggestion and one comment about using Set instead of Array. I've seen in some tests that may be duplicated file paths based on the length of the arrays.

__tests__/search.test.ts Outdated Show resolved Hide resolved
__tests__/search.test.ts Outdated Show resolved Hide resolved
__tests__/search.test.ts Outdated Show resolved Hide resolved
Comment on lines +34 to +35
const commonPaths = new Array<string>()
const splitPaths = new Array<string[]>()
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think on using Sets here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I try to avoid multi-dimensional arrays but i found it the easiest to use here. We're also guaranteed beforehand that all the searchpaths are unique so the main benefit of using a set is not needed.

konradpabjan and others added 2 commits July 9, 2020 18:17
Co-authored-by: Alberto Gimeno <gimenete@users.noreply.github.com>
@konradpabjan konradpabjan merged commit f265ac5 into master Jul 9, 2020
@konradpabjan konradpabjan deleted the konradpabjan/multi-path-upload branch July 9, 2020 18:54
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.

Exclude files/dirs either by name or regexp
3 participants