Add possibility to specify images to cache #575
Conversation
There was a problem hiding this comment.
This is a good concept and has the backing of multiple previous independent feature requests. Admittedly those were already addressed by filtering out pre-existing images, but in any case I am sold on this feature being worthy of inclusion at this point. Let me know if you need any help addressing the issues detected by CI after taking a look at our contributing guide. You will be able to address the security vulnerabilities simply by rebasing.
| description: > | ||
| If "all", all images will be included. | ||
| Otherwise, specify precisely all needed images to be included separated with spaces. | ||
| required: true |
There was a problem hiding this comment.
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.
| @@ -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 }); | |||
There was a problem hiding this comment.
I don't see the rationale for specifying this argument as required.
| (image: string): boolean => !preexistingImages.includes(image) | ||
| (image: string): boolean => | ||
| !preexistingImages.includes(image) && | ||
| includedImagesList.includes(image) |
There was a problem hiding this comment.
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.
| info("newImagesArgs"); | ||
| info(newImagesArgs); |
There was a problem hiding this comment.
Was this left over from debugging? execBashCommand already logs the command passed to it, which contains this string.
| @@ -11,6 +11,12 @@ inputs: | |||
| are not supported, because partial cache restoration leads to a "snowball" | |||
| effect. | |||
| required: true | |||
| included-images: | |||
| description: > | |||
| If "all", all images will be included. | |||
There was a problem hiding this comment.
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.
| @@ -11,6 +11,12 @@ inputs: | |||
| are not supported, because partial cache restoration leads to a "snowball" | |||
| effect. | |||
| required: true | |||
| included-images: | |||
There was a problem hiding this comment.
I like the proposed name. Just gotta document it in the README too.
| @@ -11,6 +11,12 @@ inputs: | |||
| are not supported, because partial cache restoration leads to a "snowball" | |||
There was a problem hiding this comment.
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.)
No description provided.