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.

Sign up for GitHub

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

Add possibility to specify images to cache #575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add possibility to specify images to cache #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ inputs:
are not supported, because partial cache restoration leads to a "snowball"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit (commit message): the scope should be "action" rather than "ci" since "ci" within this repository would refer to the CI pipeline for this action itself. (In fairness, this action is run as part of its own CI... he he.)

effect.
required: true
included-images:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the proposed name. Just gotta document it in the README too.

description: >
If "all", all images will be included.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the default to "new" and clarify that we, to quote the README: "filter out Docker images that are present before the action is run, notably those pre-cached by GitHub actions; only save Docker images pulled or built in the same job after the action is run." I don't see much reason to support "all" unless we get some use case for that from folks using self-hosted runners that aren't pre-caching Docker images.

Otherwise, specify precisely all needed images to be included separated with spaces.
required: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be missing something, but I think this should be false since, when not specified, the default behavior ought to be the behavior that predates this PR.

default: "all"
read-only:
description: If "true", disables saving the cache upon cache miss
required: false
Expand Down
2 changes: 1 addition & 1 deletion dist/main/index.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/post/index.js
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion src/docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const loadDockerImages = async (): Promise<void> => {

const saveDockerImages = async (): Promise<void> => {
const key = getInput("key", { required: true });
const includedImages = getInput("included-images", { required: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the rationale for specifying this argument as required.

const includedImagesList = includedImages.split(" ");
if (getState(CACHE_HIT) === "true") {
info(`Cache hit occurred on the primary key ${key}, not saving cache.`);
} else if (getInput("read-only") === "true") {
Expand All @@ -42,7 +44,9 @@ const saveDockerImages = async (): Promise<void> => {
const images = await execBashCommand(LIST_COMMAND);
const imagesList = images.split("\n");
const newImages = imagesList.filter(
(image: string): boolean => !preexistingImages.includes(image)
(image: string): boolean =>
!preexistingImages.includes(image) &&
includedImagesList.includes(image)
Copy link
Contributor

Choose a reason for hiding this comment

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

To pass CI/CR, you will need test coverage for the (currently unhandled) case where the default value is passed as well as the case where it is overridden by the user. I think users will find it counter-intuitive if pre-existing images are still filtered out when they specified explicitly what to cache.

);
if (newImages.length === 0) {
info("No Docker images to save");
Expand All @@ -52,6 +56,8 @@ const saveDockerImages = async (): Promise<void> => {
"will be saved."
);
const newImagesArgs = newImages.join(" ");
info("newImagesArgs");
info(newImagesArgs);
Comment on lines +59 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this left over from debugging? execBashCommand already logs the command passed to it, which contains this string.

const cmd = `docker save --output ${DOCKER_IMAGES_PATH} ${newImagesArgs}`;
await execBashCommand(cmd);
await saveCache([DOCKER_IMAGES_PATH], key);
Expand Down