Skip to content

fix: resolve tar issues by updating to official @actions/cache #37

Merged
bahmutov merged 3 commits into
bahmutov:masterfrom
agilgur5:fix-tar-issues
Aug 10, 2020
Merged

fix: resolve tar issues by updating to official @actions/cache #37
bahmutov merged 3 commits into
bahmutov:masterfrom
agilgur5:fix-tar-issues

Conversation

@agilgur5

@agilgur5 agilgur5 commented Jul 19, 2020

Copy link
Copy Markdown
Contributor

Description

Commits

ci: add OS matrix to ensure compatibility

  • reproduces tar error in the examples when running ./ action on
    Windows and macOS

  • this should help find OS issues in the future like the tar one that
    was exclusive to macOS and Windows

  • unfortunately, can't be added as a test to the unit tests because
    they stub out the cache

fix: resolve tar issues by updating to official @actions/cache

Tags

Fixes #24
Fixes downstream jaredpalmer/tsdx#625 (comment)

EDIT: Also fixes #8 and closes #27 per #27 (comment)

API Differences

Can see how the @actions/cache API looks like with arrays:

Testing

Per commit messages, the unit tests can't detect this as they stub out @actions/cache.

But running the example-subfolders workflow acts as a kind of e2e test and will show errors as it's the only workflow that runs itself via ./ instead of bahmutov/npm-install@HEAD, so I added the OS matrix there.

Screenshot differences in CI

These are all screenshots taken from my fork on the example-subfolders workflow.

Before on Windows:
before-windows

Before on macOS:
before-macOS

After on Windows:
after-windows

After on macOS:
after-macOS

agilgur5 added 2 commits July 18, 2020 23:17
- reproduces tar error in the examples when running `./` action on
  Windows and macOS
- this should help find OS issues in the future like the tar one that
  was exclusive to macOS and Windows

- unfortunately, can't be added as a test to the unit tests because
  they stub out the cache
- was previously using a branch of a forked version since the official
  action was not published as a package and didn't export its
  internal functions (only ran them)
  - that version has bugs with using `tar` on Windows and macOS, making
    this action fail for matrix tests on various OSes
  - now the official version is published at: https://github.com/actions/toolkit/blob/master/packages/cache
    - and its tar handling has changed quite a bit since: https://github.com/actions/toolkit/blob/0bf9897205aa22619fd765a7f927983aae8f3d82/packages/cache/src/internal/tar.ts

- update usage and tests to new API that requires an array for
  inputPaths and restoreKeys
@agilgur5

Copy link
Copy Markdown
Contributor Author

@bahmutov could you review this?

@agilgur5

agilgur5 commented Aug 2, 2020

Copy link
Copy Markdown
Contributor Author

@bahmutov following up again...

@agilgur5

agilgur5 commented Aug 10, 2020

Copy link
Copy Markdown
Contributor Author

@bahmutov following up one more time... Been over 3 weeks without a response 😕

I didn't want to keep a fork but sounds like I'll have to...

Comment thread package.json Outdated

@bahmutov bahmutov left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I love it, can you address the tiny question about the version and I will be happy to merge

Comment thread package.json Outdated
@bahmutov bahmutov merged commit fedf95f into bahmutov:master Aug 10, 2020
@bahmutov

Copy link
Copy Markdown
Owner

@agilgur5 would you like to help me maintain this package since I have my hands full with work, and you know what you are doing

@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@agilgur5

agilgur5 commented Aug 14, 2020

Copy link
Copy Markdown
Contributor Author

@agilgur5 would you like to help me maintain this package since I have my hands full with work, and you know what you are doing

@bahmutov yea I can try my best and given I was thinking of forking as this is a useful action to DRY up code and decrease cache mistakes anyway, that would make sense.
This update also apparently changed a previous warning on a race condition to an error now (as can be seen in the failed checks in recent runs here, in TSDX, and in salsita/react-training#60 that mentioned this PR... not sure how the checks passed on my fork 🤔 ), so I've gotta add a hotfix PR to fix that as well ASAP
EDIT: see also #39 (comment)

That being said, I too have well over a full plate of work at my job, and am quite behind on OSS projects I already maintain, so can't say I'll have too much time to add or that this project would be top priority. But this doesn't get many issues either.

Also while the source code is pretty straightforward, I don't fully understand how the tests are organized right now, seemed a bit duplicative when I was making these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tar issues on Windows and macOS 400mb limit is no longer needed

2 participants