Cache Box PHAR in reusable binary workflow - #206
Conversation
jrfnl
left a comment
There was a problem hiding this comment.
@rahimahisah17 Thank you for this PR and your willingness to contribute to PHP Parallel Lint!
I've reviewed the PR and have left some feedback in line.
Additionally:
- I believe that this workflow will now need a
use-cache"input" which defaults totrue, but can be passed asfalsefrom calling workflows, and should be respected in the workflow steps.
The reason for this, is that this job is also used in therelease.ymlworkflow and I wouldn't want to run the risk of a potentially poisoned cache infecting our release, so, while thetestworkflow could use the cache, thereleaseworkflow should definitely not do so. - I believe that, to optimize the caching, it may be beneficial to not make the caching dependent on the rest of the workflow succeeding.
See for more info: https://github.com/actions/cache/blob/main/save/README.md#always-save-cache - Lastly, I'd like to ask you whether you have tested the PR ? I mean, I can see the cache being created in the workflow run from this PR, but what I cannot see (or confirm via proof at this moment) is a cache having been created and then being re-used.
I'd also like to ask you why the commit was created by "DevUser" and not you ?
I know it's a lot of feedback. Please do not let this discourage you and feel free to ask questions if anything is unclear. I'd rather you ask me than ask an LLM (which I suspect you used ?) as they get too much wrong.
| # Note: do NOT turn on the requirement checker in the box config as it is no longer | ||
| # compatible with PHP < 7.2. |
There was a problem hiding this comment.
This comment should be moved down (or rather, the new code should be moved up) as it belongs with the "Install box"/"Validate configuration" steps.
The comment in its current place doesn't make sense anymore.
There was a problem hiding this comment.
Thanks for taking the time to review this PR and for the detailed feedback.
I understand the points raised in the review. I’ll go through the inline comments and make the necessary adjustments to the workflow, particularly around the Box version detection, cache key handling, cache hit/miss behavior, and the download/permission steps. I’ll also make sure the comments accurately reflect the behavior of the workflow.
Regarding DevUser and dev@skill-sch.com: these values came directly from a Skill.sch Git lab that I was completing. The lab specifically instructed me to configure my global Git username as DevUser and email as dev@skill-sch.com. They were used for that lab exercise and were not intended to represent another contributor or user of this project.
I’ll address the feedback and push the updated changes so the checks can be run again.
Thanks again for the review.
| - name: Get latest Box version | ||
| id: box-version | ||
| run: | | ||
| latest_url=$(curl -Ls -o /dev/null -w '%{url_effective}' https://github.com/box-project/box/releases/latest) | ||
| echo "version=${latest_url##*/}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
I realize this mirrors the current version download of the latest release, but I wonder if we should start to take into account that the latest release may not always be the semantically latest release. This is typically something which would/could happen if a project releases for multiple majors.
Not sure whether that is applicable here at this time, but whether it is or isn't, isn't even that relevant as it could still happen in the future and if it does, we don't want to get bitten by it.
We may also want to consider limiting to the "latest of the current major", as if Box releases a new major, the configuration we use may need updates etc.
This action runner might be helpful to solve the above feedback:
https://github.com/oprypin/find-latest-tag
I've used it successfully in another repo.
Note: if the above suggestions get implemented, the "Install box" step will also need to be updated to download the PHAR based on the version number, rather than on latest.
|
|
||
| - name: Cache Box PHAR | ||
| id: cache-box | ||
| uses: actions/cache@v4 |
There was a problem hiding this comment.
Version is very much out of date. Please update.
Note: Updating the action runner should also get rid of the warnings about the use of Node 20...
There was a problem hiding this comment.
Thanks for the detailed review. I’ve addressed the requested changes, including the use-cache input, disabling the cache for the release workflow, separating cache restore/save, and moving the Box compatibility comment. I also corrected the Git author information and verified the updated workflows pass the checks.
The changes are now pushed to the PR and ready for another review. Thanks again!
8cf822f to
1483ce1
Compare
Description
Cache the Box PHAR used by the reusable binary build workflow.
Changes
box.pharusingactions/cache@v4.Related issue
Closes #200