fix: filter out anonymous images #789

parkedwards · 2024-03-01T21:45:12Z

relates to #572

through local testing, i'm finding that using docker save {{ .ID }} actually anonymizes named images upon docker load

given this docker client behavior, i'm thinking we might just want to filter out the anonymous images (these aren't being cached currently anyway, due to the docker client not accepting <none>:<none> as a save-able image input)

I dont see the tests running in CI/CD, so here's an output of npm run test

npm run test

> [email protected] test
> yarn run tsc && yarn run jest:esm

(node:92973) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  src/main.test.ts
(node:92972) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  src/post.test.ts
(node:92971) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  src/util.test.ts
(node:92969) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  src/docker.test.ts
(node:92970) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  src/integration.test.ts
-----------------|---------|----------|---------|---------|-------------------
File             | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-----------------|---------|----------|---------|---------|-------------------
All files        |     100 |      100 |     100 |     100 |
 src             |     100 |      100 |     100 |     100 |
  docker.ts      |     100 |      100 |     100 |     100 |
  main.ts        |     100 |      100 |     100 |     100 |
  post.ts        |     100 |      100 |     100 |     100 |
  util.ts        |     100 |      100 |     100 |     100 |
 src/arbitraries |     100 |      100 |     100 |     100 |
  util.ts        |     100 |      100 |     100 |     100 |
 src/mocks       |     100 |      100 |     100 |     100 |
  util.ts        |     100 |      100 |     100 |     100 |
-----------------|---------|----------|---------|---------|-------------------

Test Suites: 5 passed, 5 total
Tests:       11 passed, 11 total
Snapshots:   0 total
Time:        2.921 s, estimated 5 s
Ran all test suites.

Kurt-von-Laven

I would love feedback from users regarding whether this change would be beneficial to them. As a user, I would personally prefer that the action fail so that I realize I forgot to name and tag my Docker images. Clearly we could fail more gracefully than we currently do, but my initial concern is that this approach is too clever and will prevent downstream issues from being detected. I have reviewed the change in any case so that it's clear to everyone what would need to change if there is a clear user preference for silently ignoring anonymous Docker images. Any context anyone can offer around how and when such images most often arise would be greatly appreciated.

Kurt-von-Laven · 2024-03-08T11:53:12Z

src/docker.ts

@@ -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>",


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.

Kurt-von-Laven · 2024-03-08T12:02:24Z

src/docker.test.ts

+        newImages.length === 0 ||
+        newImages.every((img) => img == "<none>:<none>")


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

Kurt-von-Laven · 2024-03-08T12:04:55Z

src/docker.test.ts

@@ -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>"]]],


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.

Kurt-von-Laven · 2024-03-08T12:06:21Z

README.md

+`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>
+```
+


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.

Kurt-von-Laven · 2024-03-08T12:07:50Z

README.md

@@ -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
+
+Anonymous images (eg. `<none>:<none>`) will be skipped during caching


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

Kurt-von-Laven · 2024-03-08T12:12:45Z

README.md

+Be sure to name + tag those images you wish to be handled by the caching
+and loading operations in this action.


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

Kurt-von-Laven · 2024-03-08T12:14:07Z

README.md

+Be sure to name + tag those images you wish to be handled by the caching
+and loading operations in this action.


This can be combined with the first paragraph.

Kurt-von-Laven · 2024-03-08T12:15:29Z

README.md

@@ -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


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

Kurt-von-Laven · 2024-03-08T12:21:13Z

.gitignore

@@ -18,3 +18,5 @@ build
 !.yarn/releases
 !.yarn/sdks
 !.yarn/versions
+
+node_modules/


Can you elaborate on the rationale for this change?

Kurt-von-Laven · 2024-03-08T12:22:06Z

README.md

+      - [`key`](#key)
    - [Optional](#optional)
+      - [`read-only`](#read-only)


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

Kurt-von-Laven · 2024-03-08T12:31:51Z

Our contributing guide explains how to run our CI locally. That plus a rebase will likely get you through the remaining build errors.

Kurt-von-Laven · 2024-03-08T20:53:23Z

Actually, it occurs to me that we could probably save the anonymous images by ID and the named ones by name to avoid the need for filtering? I think we may even be able to pull this off by using conditional expressions in the format string that we pass on the command line. Concretely, I am imagining that the only production change needed may be to set:

LIST_COMMAND = `docker image list --format '{{ if ne .Repository "<none>" }}{{ .Repository }}{{ if ne .Tag "<none>" }}:{{ .Tag }}{{ end }}{{ else }}{{ .ID }}{{ end }}'`

This will cause us to use the image name and tag when available, but fall back on the ID otherwise. I am assuming that Docker disallows tagging of anonymous images, but if it's possible to have an image with a tag but no name, then the template would need to be modified to handle that case. Here are the pertinent Docker docs, which link to the Go template syntax.

Kurt-von-Laven · 2024-03-27T05:44:20Z

Superseded by #816, where I have implemented the strategy described in my previous comment.

parkedwards added 2 commits March 1, 2024 13:44

fix: filter out anonymous images

cf27fdf

test

653c0e0

parkedwards marked this pull request as ready for review March 1, 2024 22:15

lint

6f5efaa

Kurt-von-Laven self-requested a review March 8, 2024 11:42

Kurt-von-Laven mentioned this pull request Mar 8, 2024

none tags will cause save to fail #609

Closed

Kurt-von-Laven requested changes Mar 8, 2024

View reviewed changes

Kurt-von-Laven closed this Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: filter out anonymous images #789

fix: filter out anonymous images #789

parkedwards commented Mar 1, 2024 •

edited

Loading

Kurt-von-Laven left a comment

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024 •

edited

Loading

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024

Kurt-von-Laven Mar 8, 2024 •

edited

Loading

Kurt-von-Laven commented Mar 8, 2024

Kurt-von-Laven commented Mar 8, 2024 •

edited

Loading

Kurt-von-Laven commented Mar 27, 2024

		newImages.length === 0 \|\|
		newImages.every((img) => img == "<none>:<none>")

		Be sure to name + tag those images you wish to be handled by the caching
		and loading operations in this action.

fix: filter out anonymous images #789

fix: filter out anonymous images #789

Conversation

parkedwards commented Mar 1, 2024 • edited Loading

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kurt-von-Laven Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kurt-von-Laven Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Kurt-von-Laven commented Mar 8, 2024

Kurt-von-Laven commented Mar 8, 2024 • edited Loading

Kurt-von-Laven commented Mar 27, 2024

parkedwards commented Mar 1, 2024 •

edited

Loading

Kurt-von-Laven Mar 8, 2024 •

edited

Loading

Kurt-von-Laven Mar 8, 2024 •

edited

Loading

Kurt-von-Laven commented Mar 8, 2024 •

edited

Loading