Pack should support run image extension on Platform API 0.12 #1698

natalieparellano · 2023-04-03T21:10:14Z

Signed-off-by: Natalie Arellano <narellano@vmware.com>

look in <layers>/generated/build for Dockerfiles instead of looking in the order. Extensions could have been present in the order, but not detected. Signed-off-by: Natalie Arellano <narellano@vmware.com>

…ayers>/analyzed.toml and check if extend is true. Signed-off-by: Natalie Arellano <narellano@vmware.com>

…ions Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

…irectory Signed-off-by: Natalie Arellano <narellano@vmware.com>

internal/build/lifecycle_execution.go

Signed-off-by: Natalie Arellano <narellano@vmware.com>

…te successfully <layers>/generated/build isn't guaranteed to exist Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-04-07T19:37:06Z

acceptance/acceptance_test.go

@@ -798,7 +798,7 @@ func testAcceptance(
 					it("creates builder", func() {
 						// Linux containers (including Linux containers on Windows)
 						extSimpleLayersDiffID := "sha256:b9e4a0ddfb650c7aa71d1e6aceea1665365e409b3078bfdc1e51c2b07ab2b423"
-						extReadEnvDiffID := "sha256:b49f178f802bbeb04acf4776bd41dcea8c47504ce31b06b580ec697387629cf2"
+						extReadEnvDiffID := "sha256:ab7419c5e0b1a0789bd07cef2ed0573ec6e98eb05d7f05eb95d4f035243e331c"


Changed the fixture, so the SHA changed

natalieparellano · 2023-04-07T19:38:04Z

acceptance/acceptance_test.go

+							})
+						})
+
+						when("there are run image extensions", func() {


This test won't run in CI until there's a lifecycle that supports run image extension. But I've verified with my dev lifecycle that it does the job.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-04-10T15:51:59Z

Personally I think Codecov is being too picky, but I'll see about adding some more tests. I think this is ready for feedback now

joe-kimmel-vmw · 2023-04-10T20:42:29Z

acceptance/assertions/lifecycle_output.go

-	l.assert.ContainsAll(l.output, "[detector]", "[analyzer]", "[extender]", "[exporter]")
+	// Earlier pack versions print `[extender]`, later pack versions print `[extender (build)]`.
+	// Removing the `]` for the extend phase allows us to navigate compat suite complexity without undo headache.
+	// When previous pack is old enough, we can make the matcher more precise.


nice explanatory comment :)

joe-kimmel-vmw · 2023-04-10T20:54:45Z

internal/build/container_ops.go

+// CopyOutErr copies container directories to a handler function. The handler is responsible for closing the Reader.
+// CopyOutErr differs from CopyOut in that it does not require the source files to exist in the container
+// in order to exit without error.
+func CopyOutErr(handler func(closer io.ReadCloser) error, srcs ...string) ContainerOperation {


nit but also the two hardest things in computer science, amirite ? I had to read your carefully written note to understand what this name means.

What are the options?

i think there's an existing go idiom where you could rename the other function to MustCopyOut and then call this one CopyOut ? which of course would only endlessly confuse people who used to know the behavior/contract of the old function, i.e. the existing maintainers...

i think in some other languages with paradigms for optional types there's conventions around putting words like "maybe" to indicate that something might return what you asked for but it's the caller's job to check. To make that explicit in go could we add "permits" e.g. CopyOutPermitsErr ? is there some other word?

it's possible that this is a golang idiom; feel free to tell me I'm wrong.

I added a test to this branch/PR d6aedda to try and cover this. But I didn't understand the statement well enough to write a test "does not require the source files to exist in the container in order to exit without error".

I think maybe CopyOutMaybe is a better name? I'd like a test for this too, but I'm struggling to understand it.

CopyOutMaybe sounds good to me - updated in 24ce465 along with the comment to hopefully be more clear

internal/build/lifecycle_execution.go

Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Also test that -stack is provided for older platforms Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-04-13T20:27:38Z

Green!

github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Apr 3, 2023

github-actions bot added this to the 0.30.0 milestone Apr 3, 2023

natalieparellano added 5 commits April 5, 2023 16:51

Add extend for run

99c0cbc

Signed-off-by: Natalie Arellano <narellano@vmware.com>

When deciding whether to pull the builder manifest or run the extender,

791ae2f

look in <layers>/generated/build for Dockerfiles instead of looking in the order. Extensions could have been present in the order, but not detected. Signed-off-by: Natalie Arellano <narellano@vmware.com>

When deciding whether to run the extender (for run image), look in <l…

ea4b3a5

…ayers>/analyzed.toml and check if extend is true. Signed-off-by: Natalie Arellano <narellano@vmware.com>

Restorer should provided /kaniko bind when there are run image extens…

cbd7dd1

…ions Signed-off-by: Natalie Arellano <narellano@vmware.com>

Look for build Dockerfiles in the right directory

46578c1

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/pack-build-1620 branch from 4f16428 to 46578c1 Compare April 6, 2023 13:47

natalieparellano added 2 commits April 6, 2023 10:13

Fix container name

5ae6791

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Rename field to avoid confusion with the lifecycle binary's working d…

8863809

…irectory Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/pack-build-1620 branch from 4ee84fb to 8863809 Compare April 6, 2023 14:18

natalieparellano commented Apr 6, 2023

View reviewed changes

internal/build/lifecycle_execution.go Outdated Show resolved Hide resolved

natalieparellano added 2 commits April 6, 2023 10:24

Lifecycle execution tests should not require Docker

e8d47ae

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Merge branch 'main' into runext/pack-build-1620

d9b7132

github-actions bot added the type/chore Issue that requests non-user facing changes. label Apr 6, 2023

Don't require file to exist in container for copy operation to comple…

1ba8367

…te successfully <layers>/generated/build isn't guaranteed to exist Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/pack-build-1620 branch from cde918b to 1ba8367 Compare April 6, 2023 16:01

natalieparellano mentioned this pull request Apr 6, 2023

pack build support --extension #1651

Merged

2 tasks

Fixes and acceptance test

0861bc7

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/pack-build-1620 branch from 925077c to 0861bc7 Compare April 7, 2023 19:35

Merge branch 'main' into runext/pack-build-1620

b85bb08

natalieparellano commented Apr 7, 2023

View reviewed changes

natalieparellano marked this pull request as ready for review April 7, 2023 19:39

natalieparellano requested review from a team as code owners April 7, 2023 19:39

Try to fix Windows cleanup

d83204c

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/pack-build-1620 branch from 7901654 to d83204c Compare April 7, 2023 20:26

Update sha for WCOW extension

98bd954

Signed-off-by: Natalie Arellano <narellano@vmware.com>

joe-kimmel-vmw reviewed Apr 10, 2023

View reviewed changes

jkutner and others added 5 commits April 13, 2023 09:52

Added a test for build.CopyOutErr

d6aedda

Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Rename func and clarify comment

24ce465

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Provide -run to exporter for newer platforms

e8c869a

Also test that -stack is provided for older platforms Signed-off-by: Natalie Arellano <narellano@vmware.com>

Make tmpDir private by passing it into the constructor

15e52db

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Merge branch 'main' into runext/pack-build-1620

92d0380

jkutner approved these changes Apr 13, 2023

View reviewed changes

jkutner merged commit 2e1320b into main Apr 13, 2023

jkutner deleted the runext/pack-build-1620 branch April 13, 2023 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pack should support run image extension on Platform API 0.12 #1698

Pack should support run image extension on Platform API 0.12 #1698

natalieparellano commented Apr 3, 2023 •

edited

Loading

natalieparellano Apr 7, 2023

natalieparellano Apr 7, 2023

natalieparellano commented Apr 10, 2023

joe-kimmel-vmw Apr 10, 2023

joe-kimmel-vmw Apr 10, 2023

jkutner Apr 13, 2023

natalieparellano Apr 13, 2023

natalieparellano commented Apr 13, 2023

Pack should support run image extension on Platform API 0.12 #1698

Pack should support run image extension on Platform API 0.12 #1698

Conversation

natalieparellano commented Apr 3, 2023 • edited Loading

natalieparellano Apr 7, 2023

Choose a reason for hiding this comment

natalieparellano Apr 7, 2023

Choose a reason for hiding this comment

natalieparellano commented Apr 10, 2023

joe-kimmel-vmw Apr 10, 2023

Choose a reason for hiding this comment

joe-kimmel-vmw Apr 10, 2023

Choose a reason for hiding this comment

jkutner Apr 13, 2023

Choose a reason for hiding this comment

natalieparellano Apr 13, 2023

Choose a reason for hiding this comment

natalieparellano commented Apr 13, 2023

natalieparellano commented Apr 3, 2023 •

edited

Loading