perf(action): Filter out pre-cached images #98

mwarres · 2022-08-02T01:40:02Z

Fixes #45.

Only save relevant Docker images. Improve performance when run as root, e.g., in Test workflow, the Save Cache job ran for approximately 5 minutes and now runs in less than 10 seconds. The Restore Cache job ran in approximately 40 seconds and now runs in less than 10 seconds. Previously, unnecessary Docker images were saved because GitHub Actions pre-caches a standard set of Docker images when Docker runs as root; those images aren't accessible when using rootless-docker, so this change doesn't impact users of rootless-docker.

Kurt-von-Laven

Good work! It's probably worth another pass with fresh eyes after addressing review feedback to double check that all of the proper assertions were actually added since tests don't fail by themselves.

src/docker.test.ts

src/docker.ts

src/integration.test.ts

src/docker.ts

src/integration.test.ts

src/docker.test.ts

Kurt-von-Laven

Lots of great progress here. Good to see more asserts working their way in. This is getting close to landing.

src/docker.test.ts

src/docker.ts

src/integration.test.ts

src/docker.test.ts

src/integration.test.ts

src/docker.test.ts

Kurt-von-Laven

Good progress!

src/docker.test.ts

src/integration.test.ts

src/docker.test.ts

src/integration.test.ts

mwarres

Note: I haven't divided the PR into separate PRs yet but will do so after the other issues with this PR are addressed satisfactorily.

src/docker.test.ts

src/integration.test.ts

src/docker.test.ts

src/integration.test.ts

src/docker.test.ts

Kurt-von-Laven · 2022-08-10T03:42:31Z

Consider quantifying the approximate performance improvement of this change on the test workflow in the commit message.

Kurt-von-Laven

Commit message: when documenting performance it's useful it say the approximate runtime before and after the change since there's a huge difference between making a 1-hour job 1 minute faster and making a 2-minute job 1 minute faster. Consider "in test workflow: the" --> "in the test workflow, the" (or "in Test workflow, the").

src/integration.test.ts

src/docker.test.ts

src/docker.ts

poetry.lock

src/docker.ts

src/docker.test.ts

src/integration.test.ts

src/util.ts

src/docker.test.ts

src/docker.ts

src/integration.test.ts

src/docker.test.ts

src/util.test.ts

src/docker.test.ts

src/util.test.ts

src/integration.test.ts

src/docker.test.ts

src/integration.test.ts

Only save relevant Docker images. Improve performance when run as root, e.g., in Test workflow, the Save Cache job ran for approximately 5 minutes and now runs in less than 10 seconds. The Restore Cache job ran in approximately 40 seconds and now runs in less than 10 seconds. Previously, unnecessary Docker images were saved because GitHub Actions pre-caches a standard set of Docker images when Docker runs as root; those images aren't accessible when using rootless-docker, so this change doesn't impact users of rootless-docker.

Kurt-von-Laven

LGTM No further questions your honor. Congratulations on finishing this one!

mwarres requested a review from Kurt-von-Laven August 2, 2022 01:40

mwarres self-assigned this Aug 2, 2022

Kurt-von-Laven requested changes Aug 2, 2022

View reviewed changes

Kurt-von-Laven reviewed Aug 2, 2022

View reviewed changes

src/docker.test.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from 718e7ea to f32380b Compare August 4, 2022 00:14

mwarres requested a review from Kurt-von-Laven August 4, 2022 00:18

Kurt-von-Laven requested changes Aug 4, 2022

View reviewed changes

mwarres force-pushed the action branch from f32380b to 4dba811 Compare August 4, 2022 23:16

Kurt-von-Laven requested changes Aug 5, 2022

View reviewed changes

mwarres force-pushed the action branch from 4dba811 to 0f0e7e6 Compare August 7, 2022 01:06

mwarres changed the title ~~perf(action): Eliminate unnecessary caching~~ perf(action): Filter out pre-cached images Aug 7, 2022

mwarres requested a review from Kurt-von-Laven August 7, 2022 01:10

Kurt-von-Laven requested changes Aug 8, 2022

View reviewed changes

mwarres force-pushed the action branch from 0f0e7e6 to 872f1ae Compare August 8, 2022 23:24

mwarres commented Aug 8, 2022

View reviewed changes

src/docker.test.ts Outdated Show resolved Hide resolved

src/integration.test.ts Show resolved Hide resolved

mwarres requested a review from Kurt-von-Laven August 8, 2022 23:28

Kurt-von-Laven requested changes Aug 9, 2022

View reviewed changes

mwarres force-pushed the action branch from 872f1ae to 1b9a9e1 Compare August 10, 2022 17:40

mwarres requested a review from Kurt-von-Laven August 10, 2022 17:42

Kurt-von-Laven requested changes Aug 10, 2022

View reviewed changes

src/integration.test.ts Show resolved Hide resolved

mwarres force-pushed the action branch from 1b9a9e1 to 71ef325 Compare August 10, 2022 21:21

mwarres requested a review from Kurt-von-Laven August 10, 2022 21:22

Kurt-von-Laven requested changes Aug 10, 2022

View reviewed changes

src/docker.test.ts Show resolved Hide resolved

src/docker.test.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from 71ef325 to 11cc540 Compare August 10, 2022 22:31

mwarres requested a review from Kurt-von-Laven August 10, 2022 22:33

mwarres force-pushed the action branch from 11cc540 to 711236d Compare August 11, 2022 00:06

Kurt-von-Laven requested changes Aug 11, 2022

View reviewed changes

src/docker.ts Outdated Show resolved Hide resolved

src/docker.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from 711236d to 4bda6ad Compare August 11, 2022 22:13

mwarres requested a review from Kurt-von-Laven August 11, 2022 22:13

mwarres force-pushed the action branch 2 times, most recently from b37a43a to c4f1b66 Compare August 23, 2022 15:19

Kurt-von-Laven requested changes Aug 24, 2022

View reviewed changes

Kurt-von-Laven requested changes Aug 26, 2022

View reviewed changes

src/docker.test.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from c4f1b66 to 4c3c871 Compare August 27, 2022 01:11

Kurt-von-Laven requested changes Aug 27, 2022

View reviewed changes

src/docker.test.ts Show resolved Hide resolved

mwarres force-pushed the action branch from 4c3c871 to ad0cb8f Compare August 27, 2022 20:45

mwarres requested a review from Kurt-von-Laven August 27, 2022 21:38

Kurt-von-Laven requested changes Aug 28, 2022

View reviewed changes

mwarres force-pushed the action branch 2 times, most recently from c878b70 to 5bb6ebc Compare August 29, 2022 04:25

mwarres requested a review from Kurt-von-Laven August 29, 2022 04:29

Kurt-von-Laven requested changes Aug 29, 2022

View reviewed changes

src/integration.test.ts Outdated Show resolved Hide resolved

src/docker.test.ts Show resolved Hide resolved

src/util.test.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from 5bb6ebc to 6541dac Compare August 29, 2022 21:23

mwarres requested a review from Kurt-von-Laven August 29, 2022 21:24

mwarres force-pushed the action branch from 6541dac to 8da0525 Compare August 29, 2022 22:35

Kurt-von-Laven requested changes Aug 29, 2022

View reviewed changes

src/docker.test.ts Show resolved Hide resolved

src/docker.test.ts Outdated Show resolved Hide resolved

src/util.test.ts Outdated Show resolved Hide resolved

src/util.test.ts Outdated Show resolved Hide resolved

mwarres force-pushed the action branch from 8da0525 to 9c23a17 Compare August 30, 2022 17:28

mwarres requested a review from Kurt-von-Laven August 30, 2022 20:10

Kurt-von-Laven requested changes Aug 31, 2022

View reviewed changes

src/integration.test.ts Outdated Show resolved Hide resolved

src/integration.test.ts Show resolved Hide resolved

src/integration.test.ts Show resolved Hide resolved

src/docker.test.ts Outdated Show resolved Hide resolved

mwarres requested a review from Kurt-von-Laven August 31, 2022 19:57

mwarres force-pushed the action branch from 9c23a17 to de7a63d Compare August 31, 2022 19:59

Kurt-von-Laven requested changes Aug 31, 2022

View reviewed changes

src/docker.test.ts Outdated Show resolved Hide resolved

src/integration.test.ts Show resolved Hide resolved

src/integration.test.ts Outdated Show resolved Hide resolved

src/integration.test.ts Outdated Show resolved Hide resolved

src/integration.test.ts Show resolved Hide resolved

mwarres force-pushed the action branch from de7a63d to 7a3d990 Compare September 2, 2022 04:48

mwarres requested a review from Kurt-von-Laven September 2, 2022 05:16

Kurt-von-Laven approved these changes Sep 5, 2022

View reviewed changes

mwarres merged commit dc8862c into main Sep 5, 2022

mwarres deleted the action branch September 5, 2022 06:22

InfluxOW mentioned this pull request Nov 11, 2022

Allow filter out user-defined images #279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(action): Filter out pre-cached images #98

perf(action): Filter out pre-cached images #98

mwarres commented Aug 2, 2022 •

edited by Kurt-von-Laven

Loading

Kurt-von-Laven left a comment

Kurt-von-Laven left a comment

Kurt-von-Laven left a comment

mwarres left a comment

Kurt-von-Laven commented Aug 10, 2022 •

edited

Loading

Kurt-von-Laven left a comment

Kurt-von-Laven left a comment

perf(action): Filter out pre-cached images #98

perf(action): Filter out pre-cached images #98

Conversation

mwarres commented Aug 2, 2022 • edited by Kurt-von-Laven Loading

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

mwarres left a comment

Choose a reason for hiding this comment

Kurt-von-Laven commented Aug 10, 2022 • edited Loading

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

mwarres commented Aug 2, 2022 •

edited by Kurt-von-Laven

Loading

Kurt-von-Laven commented Aug 10, 2022 •

edited

Loading