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

fix: filter out anonymous images #789

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ build
!.yarn/releases
!.yarn/sdks
!.yarn/versions

node_modules/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on the rationale for this change?

27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@ Cache Docker Images Whether Built or Pulled
- [Usage](#usage)
- [Inputs](#inputs)
- [Required](#required)
- [`key`](#key)
- [Optional](#optional)
- [`read-only`](#read-only)
Comment on lines +30 to +32
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven Mar 8, 2024

Choose a reason for hiding this comment

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

This doesn't seem related to this pull request. I'm guessing this change was made by our tooling?

- [Outputs](#outputs)
- [`cache-hit`](#cache-hit)
- [Anonymous image names](#anonymous-image-names)
- [Supported Runners](#supported-runners)
- [Permissions](#permissions)
- [Changelog](#changelog)
Expand Down Expand Up @@ -94,6 +97,30 @@ True on cache hit (even if the subsequent
failed) and false on cache miss. See also
[skipping steps based on cache-hit](https://github.com/marketplace/actions/cache#Skipping-steps-based-on-cache-hit).

## Anonymous image names
Copy link
Contributor

Choose a reason for hiding this comment

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

"Anonymous image names" --> "Anonymous Images" (since they could either have anonymous names or tags or both)

Copy link
Contributor

Choose a reason for hiding this comment

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

This section is only relevant to a small minority of users, so let's move it below the Permissions section.


Anonymous images (eg. `<none>:<none>`) will be skipped during caching
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "eg." --> "e.g.,"

via `docker save`, as we use the `{{ .Repository }}:{{ .Tag }}` format
to reference which images to save.

`docker save {{ .ID }}` would allow us to cache/load such anonymous images.
However, the docker client loses reference to the original
`{{ .Repository }}:{{ .Tag }}` value upon `docker load` for previously
named images, resulting in:

```sh
my-image:tag
```

to become

```sh
<none>:<none>
```

Comment on lines +106 to +120
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great context for the commit message, but I doubt the typical developer cares to know our rationale, and I am sensitive to how quickly people stop reading once they feel the docs are no longer relevant.

Be sure to name + tag those images you wish to be handled by the caching
Copy link
Contributor

Choose a reason for hiding this comment

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

"name + tag" --> "name and tag" in case the plus sign might momentarily throw off some non-native speakers

and loading operations in this action.
Comment on lines +121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

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

"handled by the caching and loading operations in this action" --> "cached" for brevity

Comment on lines +121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be combined with the first paragraph.


## Supported Runners

- Tested on `ubuntu-22.04` and `windows-2022`
Expand Down
6 changes: 5 additions & 1 deletion src/docker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,10 @@ describe("Docker images", (): void => {
assertSaveCacheHit(key);
} else if (readOnly) {
assertSaveReadOnly(key);
} else if (newImages.length === 0) {
} else if (
newImages.length === 0 ||
newImages.every((img) => img == "<none>:<none>")
Comment on lines +260 to +261
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven Mar 8, 2024

Choose a reason for hiding this comment

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

This could be simplified/corrected to: newImages.some((image: string): boolean => !image.startsWith("<none>:") && !image.endsWith(":<none>")). I don't recommend using == in JavaScript.

) {
assertNoNewImagesToSave();
} else {
assertSaveCacheMiss(key, newImages);
Expand All @@ -268,6 +271,7 @@ describe("Docker images", (): void => {
["my-key", false, false, [["preexisting-image"], []]],
["my-key", false, true, [["preexisting-image"], ["new-image"]]],
["my-key", true, false, [["preexisting-image"], ["new-image"]]],
["my-key", false, false, [["preexisting-image"], ["<none>:<none>"]]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Astutely observed that this change was needed! We should probably also bias the dockerImages arbitrary to generate some unnamed and/or untagged Docker images, but I can certainly understand if you would prefer not to get into that in this PR.

],
},
);
Expand Down
3 changes: 2 additions & 1 deletion src/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ 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) && image !== "<none>:<none>",
Copy link
Contributor

Choose a reason for hiding this comment

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

The failure also occurs when only the tag is <none> (or presumably when only the image name is <none>), so we should instead check !image.startsWith("<none>:") && !image.endsWith(":<none>") && !preexistingImages.includes(image). It may be wrong in this case, but my instinct is typically to do the constant-time checks first for best performance.

);
if (newImages.length === 0) {
info("No Docker images to save");
Expand Down
Loading