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

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

Merged
merged 3 commits into from
Aug 10, 2020

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jul 19, 2020

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
Contributor Author

@bahmutov could you review this?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 2, 2020

@bahmutov following up again...

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 10, 2020

@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...

package.json Outdated Show resolved Hide resolved
Copy link
Owner

@bahmutov bahmutov left a comment

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

package.json Outdated Show resolved Hide resolved
@bahmutov bahmutov merged commit fedf95f into bahmutov:master Aug 10, 2020
@bahmutov
Copy link
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

🎉 This PR is included in version 1.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 14, 2020

@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