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(action): Verify no matching cache before save #506

Merged
merged 1 commit into from
Mar 13, 2024
Merged

feat(action): Verify no matching cache before save #506

merged 1 commit into from
Mar 13, 2024

Conversation

mwarres
Copy link
Contributor

@mwarres mwarres commented Apr 19, 2023

Fixes #135.

Prior to saving cache, verify that there is no cache with a matching key already cached. Previously, we checked if a cache key matching our cache was present in the load step; if not, we would then proceed to attempt to save the cache in the save step (assuming we were not in read-only mode and that there were indeed new Docker images to save). This was
problematic when the action was run in parallel; in that case, there were multiple, unnecessary cache save attempts when the cache initially missed but was subsequently saved.

@mwarres mwarres self-assigned this Apr 19, 2023
@mwarres
Copy link
Contributor Author

mwarres commented Apr 19, 2023

Note: this does not have appropriate testing, current status of PR is just production code changes.

Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven left a comment

Choose a reason for hiding this comment

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

This looks like a great start! I wonder if the failure in the Save Cache job can be addressed by disabling minification.

Commit message: "was indeed a new docker image(s) to save" --> "were indeed new Docker images to save"

src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
types/aliases.ts Outdated Show resolved Hide resolved
@mwarres mwarres force-pushed the action branch 3 times, most recently from f8fd9a2 to 27959c6 Compare April 20, 2023 04:29
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven left a comment

Choose a reason for hiding this comment

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

Bummer that ncc replaces cache["getCacheEntry"] with cache.getCacheEntry. We will have to copy the corresponding code from @actions/cache into our repository and modify the visibility of getCacheEntry to get this to work.

src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
@mwarres mwarres force-pushed the action branch 2 times, most recently from 0f26ad9 to a154f3d Compare March 6, 2024 02:57
src/docker.ts Show resolved Hide resolved
src/docker.ts Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.test.ts Show resolved Hide resolved
src/docker.test.ts Outdated Show resolved Hide resolved
src/docker.test.ts Outdated Show resolved Hide resolved
src/docker.test.ts Show resolved Hide resolved
src/docker.test.ts Show resolved Hide resolved
src/docker.ts Outdated Show resolved Hide resolved
src/docker.test.ts Show resolved Hide resolved
Prior to saving cache, verify that there is no cache with a matching key
already cached. Previously, we checked if a cache key matching our cache
was present in the load step; if not, we would then proceed to attempt
to save the cache in the save step (assuming we were not in read-only
mode and that there were indeed new Docker images to save). This was
problematic when the action was run in parallel; in that case, there
were multiple, unnecessary cache save attempts when the cache initially
missed but was subsequently saved.
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven left a comment

Choose a reason for hiding this comment

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

LGTM

@mwarres mwarres merged commit c50da5e into main Mar 13, 2024
8 checks passed
@mwarres mwarres deleted the action branch March 13, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Avoid Post Cache-phase when key is already in cache
2 participants