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

feat: Add force-overwrite for cache #1308

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

PrinsFrank
Copy link

@PrinsFrank PrinsFrank commented Jan 22, 2024

Description

This PR adds a force-overwrite options to force overwrite an already existing cache.

I could use some help with the implementation. I'm native to PHP and not familiar with where all the code lives in the NPM packages. There is a CLI available with this exact command, but I guess it would make sense to implement the request to delete a cache in typescript just as all the other request, but I don't know where to. Any pointer or help would be appreciated!

Motivation and Context

When working with very long workflows and incremental changes any tiny change in cache can be helpful. That's why currently I delete a cache when it exists with the following workflow:

- name: Pull PHPStan cache
  id: pull-phpstan-cache
  uses: actions/cache/restore@v3
  with:
    path: ${{ env.PHPSTAN_CACHE_DIR }}
    key: phpstan-${{ github.ref_name }}
    restore-keys: |
      phpstan

- name: Run PHPStan
  run: composer run stan

- name: Install gh-actions cache plugin
  if: always()
  run: gh extension install actions/gh-actions-cache

- name: Clear previous version of PHPStan branch cache
  if: always()
  continue-on-error: true
  run: gh actions-cache delete phpstan-${{ github.ref_name }} -B ${{ github.ref }} --confirm

- name: Push PHPStan branch cache
  if: always()
  uses: actions/cache/save@v3
  with:
    path: ${{ env.PHPSTAN_CACHE_DIR }}
    key: phpstan-${{ github.ref_name }}

For context: without any caching the job will take about 20 minutes. With a main cache and a 'branch cache' that is never updated again, the job will take a few minutes after the first commit, but every next change results in the cache being further out of date and the job taking longer and longer. That's why deleting the existing cache will speed things up significantly.

How Has This Been Tested?

N/A

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (add or update README or docs)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PrinsFrank PrinsFrank requested a review from a team as a code owner January 22, 2024 17:59
@PrinsFrank PrinsFrank changed the title Add force-overwrite for cache feat: Add force-overwrite for cache Jan 22, 2024
Copy link

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

want

@PrinsFrank
Copy link
Author

@joshmgross Would you mind reviewing this, or could you point me to the right person for that? This would close #342 which has hundreds of upvotes, mentions and a lot of comments.

Copy link

@szepeviktor szepeviktor left a comment

Choose a reason for hiding this comment

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

Many need this.

src/saveImpl.ts Outdated
);
return;
}

if (utils.isExactKeyMatch(primaryKey, restoredKey) && forceOverwrite) {
await issueCommand('actions-cache delete', {}, primaryKey);
Copy link
Member

Choose a reason for hiding this comment

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

@PrinsFrank were you able to validate these changes in your own repository?

I'm not entirely sure if the backend supporting the cache action will allow overwriting an existing cache entry like this, and as far as I know this actions-cache delete command is not a real command.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I got confused with the CI options that are available in the actions-cache plugin, but not in the toolkit yet. I use that here: https://github.com/PrinsFrank/CI-PHP/blob/main/.github/workflows/quality.yml#L134, so I can confirm this workflow works, the code just currently doesn't. I wasn't sure how to run this code, but I figured it out and can see this doesn't work in the current form.

If I understand correctly there's currently not any code for this at all in upstream packages, so I tried implementing it in actions/toolkit#1783. As I mentioned before, I'm not a typescript developer so any pointers would be very much appreciated.

@PrinsFrank PrinsFrank force-pushed the add-force-overwrite-for-cache branch 3 times, most recently from e457006 to 5afc636 Compare August 2, 2024 17:29
@PrinsFrank PrinsFrank force-pushed the add-force-overwrite-for-cache branch from 5afc636 to 0d93be3 Compare August 2, 2024 19:12
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.

4 participants