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

Implement copy and copy-immutable strategies #20

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

oNaiPs
Copy link
Contributor

@oNaiPs oNaiPs commented Nov 15, 2023

Follow-up to #19.

Adds a new strategy key that allows user to specify how cache is copied/moved.
Default strategy is called move, which was already implemented.
Copy strategy does a recursive copy to/from the cache directory on job end/start.
Copy-immutable strategy works the same way, but does not refresh cache if it already exists.

Fixes #15

TODO @stefanmaric any way we can run the actions before merging?

@oNaiPs oNaiPs force-pushed the cache_strategy branch 5 times, most recently from 6aa5293 to 64bc070 Compare November 15, 2023 15:38
src/lib/getVars.ts Outdated Show resolved Hide resolved
src/lib/getVars.ts Outdated Show resolved Hide resolved
src/post.ts Outdated
Comment on lines 14 to 32

switch (options.strategy) {
case Strategy.CopyImmutable:
if (await exists(cachePath)) {
log.info(`Cache already exists, skipping`)
return
}
await cp(targetPath, cachePath, { copySourceDirectory: true, recursive: true })
break
case Strategy.Copy:
await rmRF(cachePath)
await cp(targetPath, cachePath, { copySourceDirectory: true, recursive: true })
break
case Strategy.Move:
await mv(targetPath, cachePath, { force: true })
break
}

log.info(`Cache saved to ${cachePath} with ${options.strategy} strategy`)
Copy link
Member

Choose a reason for hiding this comment

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

Nice touch adding some feedback about what's happening. We should make it consistent, tho.

In the last log it is saying CACHE SAVED even if it just said CACHE EXISTS SO WE SKIPPING.

I would suggest to add a log line inside each case. For the copy-immutable one you would log either SKIPPED or SAVED based on the condition you have there already.

Copy link
Contributor Author

@oNaiPs oNaiPs Nov 16, 2023

Choose a reason for hiding this comment

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

@stefanmaric on line 18, it returns immediately, so it should not print the CACHE SAVED line.
I can add a log line to each one however it will be a bit redundant (3x same log)?

Copy link

Choose a reason for hiding this comment

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

Thank you @oNaiPs ! Need this change so much, makes a lot of sense for my use case. I hope @stefanmaric will have some time to approve this :)

Choose a reason for hiding this comment

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

Yesterday I got error EXDEV: cross-device link not permitted when adding action-local-cache to my project. Many thank you to @oNaiPs, you fixes saved my life! 👍

README.md Outdated Show resolved Hide resolved
@stefanmaric
Copy link
Member

Thanks for following up @oNaiPs! ❤️

TODO @stefanmaric any way we can run the actions before merging?

Once the PR has been approved, Github will allow me to run the CI based on your branch.

@oNaiPs oNaiPs force-pushed the cache_strategy branch 2 times, most recently from 278a3ca to eb347d7 Compare November 22, 2023 16:14
@oNaiPs
Copy link
Contributor Author

oNaiPs commented Nov 22, 2023

@stefanmaric please check the outstanding comment, the others have been addressed

Copy link
Member

@stefanmaric stefanmaric left a comment

Choose a reason for hiding this comment

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

Looking pretty good! Thanks for your contribution @oNaiPs ❤️

@stefanmaric
Copy link
Member

@oNaiPs tests are failing for all strategies at the cache check steps. Test cases look good to me. Logs for the action look good as well. Implementation looks good as well. So I'm clueless at this point. We would have to debug more but I'm busy at the moment. Would you be able to setup a worker of your own to debug?

@stefanmaric
Copy link
Member

Test cases look good to me.

@oNaiPs actually, I think the test conditions are wrong.

test $(cat ./demo-output.txt) != "demo-results" && echo "Wrong cached contents $(cat ./demo-output.txt)" && exit 1

That will always return exit code 1, regardless if the two other commands execute or not.

test 'a' != 'b' && echo BAD && exit 1
# prints BAD
echo $? # prints 1

test 'a' != 'a' && echo BAD && exit 1
echo $? # prints 1

To fix this, I would make the inverse. In POSIX/Bash/etc logical operators don't behave in the same way as they do in C-like languages, so this won't work:

test 'a' = 'a' || echo BAD && exit 1
# exit with code 1

Wrapping in parentheses (a sub-shell) is necessary:

test $(cat ./demo-output.txt) = "demo-results" || (echo "Wrong cached contents $(cat ./demo-output.txt)" && exit 1)

I'm not entirely sure this will work within Github Actions because even tho the code of the command is being set to 1, we are not actually breaking the execution of the shell, because the exit is within the sub-shell. I would expect Github Action runners do check the status code after each command so worth trying. If it doesn't, we would need to fallback to if []; else; fi; syntax.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Nov 29, 2023

@stefanmaric let me look into it and come back to you

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Nov 29, 2023

@stefanmaric should be fixed now (force-pushed to same commit)!
You're right, the logic was wrong. I am now using instead a bash condition, for example:

if [ $(cat ./demo-output.txt) != "demo-results" ]; then
  echo "Wrong cached contents: $(cat ./demo-output.txt)"
  exit 1
fi

I setup a self-hosted runner and tests are now passing.

@stefanmaric
Copy link
Member

All green now @oNaiPs! Merging now. Will be releasing a new version shortly. Thanks again for your contribution.

@stefanmaric stefanmaric merged commit c1798ec into MasterworksIO:main Nov 30, 2023
9 checks passed
@TwitchBronBron
Copy link

Any updates on the release of this feature? I tried using it in v2 before noticing the strategy prop isn't actually available yet.

stefanmaric added a commit that referenced this pull request Dec 22, 2023
- Implement copy and copy-immutable strategies (#20) thanks @oNaiPs
@stefanmaric
Copy link
Member

Any updates on the release of this feature? I tried using it in v2 before noticing the strategy prop isn't actually available yet.

I wanted to release it as a minor but we needed to validate there were no breaking changes before that. We were busy so we couldn't run our internal tests before.

It is all set now: https://github.com/MasterworksIO/action-local-cache/releases/tag/2.1.0

Thanks again @oNaiPs for your contributions!

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.

doesnt work when work is different drive than cache
5 participants