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

Index entries in action cache directory by SHA #2658

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChemiCalChems
Copy link

Previously they were indexed by ref name, which could be a SHA or a tag/branch name, but if a branch name was used and that branch was updated, the new reusable workflow action wouldn't be pulled.

This meant the user would have to delete the associated cache entry to be able to use the new version of the reusable workflow. If the user really wants stability, they should specify the reusable workflow by
SHA instead of by branch name/tag.

Ready to merge on my side.

@ChemiCalChems ChemiCalChems requested a review from a team as a code owner February 10, 2025 16:48
Previously they were indexed by ref name, which
could be a SHA or a tag/branch name, but if a
branch name was used and that branch was updated,
the new reusable workflow action wouldn't be pulled
@ChemiCalChems ChemiCalChems force-pushed the reusable-workflows-use-sha branch from c3e3a15 to 1277c09 Compare February 10, 2025 16:49
@ChristopherHX
Copy link
Contributor

IMHO checking out a single repo for every sha (or even ref) and clone into independent folders is a waste of storage space, clone time and much more.

That a branch fails to update is a bug of that implementation, that should be able to update it's worktree

My newer implementation of the action cache no longer

  • has any worktree
  • has more than one clone per repository

I'm all in for removal of the whole old logic that has this defect.

planned full transition stopped for the time being, I'm neutral here you can proceed supporting this

@mergify mergify bot added the needs-work Extra attention is needed label Feb 10, 2025
@ChemiCalChems
Copy link
Author

ChemiCalChems commented Feb 10, 2025

@ChristopherHX I agree that using the new action cache would be better, but since as you said for the time being a full transition won't be happening, I believe it's important to support this. I initially faced this issue when using Gitea's action runner which is used for CI/CD, not just local running of actions, which is forked from this, and there it was a PITA to remove the cache.

I also noticed I wasn't using the run context auth token to perform the listing operation, hence the PR update.

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 60.71429% with 11 lines in your changes missing coverage. Please review.

Project coverage is 68.55%. Comparing base (5a80a04) to head (5dba7bf).
Report is 168 commits behind head on master.

Files with missing lines Patch % Lines
pkg/common/git/git.go 63.63% 6 Missing and 2 partials ⚠️
pkg/runner/reusable_workflow.go 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2658      +/-   ##
==========================================
+ Coverage   61.56%   68.55%   +6.99%     
==========================================
  Files          53       72      +19     
  Lines        9002    11070    +2068     
==========================================
+ Hits         5542     7589    +2047     
+ Misses       3020     2908     -112     
- Partials      440      573     +133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot removed the needs-work Extra attention is needed label Feb 10, 2025
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.

2 participants