Multicall launcher #370

ekcasey · 2020-08-06T22:57:37Z

Multicall launcher in platform 0.4

natalieparellano

Still reviewing this PR, but here is what I've accumulated so far...

acceptance/launcher_test.go

natalieparellano · 2020-08-07T18:22:28Z

acceptance/launcher_test.go

+	when("Platform API >= 0.4", func() {
+		when("entrypoint is a process", func() {
+			it("launches that process", func() {
+				entrypoint := "/cnb/process/web"


If I'm understanding correctly, we need to prepend /cnb/process/ to web because launchImage was not built by the exporter. This feels okay to me... but I am wondering if there's a case to be made for building the test image the same way that it would be built in the wild.

Good point, I'll add /cnb/process to the path in the Dockerfile

natalieparellano · 2020-08-07T18:24:47Z

acceptance/launcher_test.go

-		when("the process type has no args", func() {
-			it("runs command as script", func() {
-				h.SkipIf(t, runtime.GOOS == "windows", "scripts are unsupported on windows")
+		when("CNB_PROCESS_TYPE is set", func() {


Do we have any test that shows what happens when the process type is provided as a positional argument?

It shouldn't work, and I didn't necessary want to write an "it fails" test

natalieparellano · 2020-08-07T18:27:30Z

acceptance/launcher_test.go

+				cmd := exec.Command("docker", "run", "--rm",
+					"--entrypoint=/cnb/lifecycle/launcher",
+					"--env=CNB_PLATFORM_API=0.4",
+					launchImage, "--", "env")


Is there a case to be made for showing the non-direct version of a custom process?

To avoid testing the full combinatorial explosion of APIs/args/direct (we would have a lot of tests) I was going to let the platform 0.3 test handle that, knowing that we should be in the same code path at that point.

natalieparellano · 2020-08-07T18:40:55Z

launch/launcher_back_test.go

@@ -0,0 +1,592 @@
+package launch_test


Do we intend to remove this file?

yes! lol

thanks

natalieparellano · 2020-08-07T19:30:42Z

exporter_test.go

+					it("does not set CNB_PROCESS_TYPE", func() {
+						_, err := exporter.Export(opts)
+						h.AssertNil(t, err)
+
+						val, err := fakeAppImage.Env("CNB_PROCESS_TYPE")
+						h.AssertNil(t, err)
+						h.AssertEq(t, val, "")
+					})
+


I think this test is in the wrong context.

natalieparellano · 2020-08-07T19:31:47Z

exporter_test.go

-					h.AssertNil(t, err)
-					h.AssertEq(t, val, "some-process-type")
+					it("sets CNB_PROCESS_TYPE", func() {
+						opts.DefaultProcessType = "some-process-type"


Can this be moved to the before? Otherwise we might forget to set it.

natalieparellano · 2020-08-07T19:32:06Z

exporter_test.go

+					})
+
+					it("sets ENTRYPOINT to launcher", func() {
+						_, err := exporter.Export(opts)


This seems to test "no default process"

For API < 0.4 the launcher point should be launcher even when there is no default process. I implemented your suggestion to move the default process setup to the before (fixing the test and the inaccurate description here)

natalieparellano · 2020-08-07T19:48:29Z

metadata.go

@@ -61,12 +65,13 @@ func (cm *CacheMetadata) MetadataForBuildpack(id string) BuildpackLayersMetadata

 // NOTE: This struct MUST be kept in sync with `LayersMetadataCompat`


Do we need to add ProcessTypes to LayersMetadataCompat then?

I suppose it does say MUST! :)

natalieparellano

Adding a few more comments

natalieparellano · 2020-08-07T20:32:04Z

env/launch_test.go

-					"Path=some-path",
+		when("#NewLaunchEnv", func() {
+			when("process dir is the first element in PATH", func() {
+				it("strips it", func() {


Where is /cnb/lifecycle stripped from the path? (Going off of https://github.com/buildpacks/rfcs/blob/main/text/0045-launcher-arguments.md )

Good catch! missed that one

natalieparellano · 2020-08-07T20:47:04Z

layers/launcher_test.go

+					Uid:      0,
+					Gid:      0,


Is it possible to also check the permissions?

natalieparellano · 2020-08-07T20:54:52Z

launch/bash_test.go

+				process = launch.ShellProcess{
+					Script:  true,
+					Command: `printf "SOME_VAR: '%s'" "$SOME_VAR"`,
+					Caller:  "some-profile-argv0",


This caller name seems a little strange outside of the profiles context.

At first I called it ProfileArgv0 but that isn't true in cmd. I think it is no more confusing than self but tbf that was already a confusing name

natalieparellano · 2020-08-07T20:55:58Z

launch/bash_test.go

+					Script:  true,
+					Command: `printf "SOME_ARG: '%s'" "$1"`,
+					Args:    []string{"", "some arg1"},
+					Caller:  "some-profile-argv0",


Same comment as above.

natalieparellano · 2020-08-07T20:57:09Z

launch/bash_test.go

+				process = launch.ShellProcess{
+					Script:  true,
+					Command: `printf "SOME_ARG: '%s'" "$1"`,
+					Args:    []string{"", "some arg1"},


Is the point of including the empty arg that it should be ignored? Maybe it's worth commenting about it in the it?

Functionally it doesn't matter to this test we could have printed $0 and dropped the empty first arg. But because $0 is supposed to represent the process being invoked it feels weird to put an actual argument there. We could change $0 to bash to make it more realistic/readable.

natalieparellano · 2020-08-07T20:58:35Z

launch/bash_test.go

+					Script:  false,
+					Command: `printf`,
+					Args:    []string{"SOME_VAR: '%s'", "$SOME_VAR"},
+					Caller:  "some-profile-argv0",


Same comment :)

natalieparellano · 2020-08-07T21:04:02Z

launch/process_test.go

+						})
+					})
+
+					when("cmd contains more than one arg and does not start with --", func() {


I'm a little confused... what happens when the cmd contains exactly one arg? Is that covered by these tests?

one vs many doesn't matter to cmd right now. I'll update the whens

launch/process_test.go

natalieparellano · 2020-08-07T21:36:40Z

launch/launcher_test.go

+
+						when("linux", func() {
+							it.Before(func() {
+								h.SkipIf(t, runtime.GOOS == "windows", "windows test")


Suggested change

h.SkipIf(t, runtime.GOOS == "windows", "windows test")

h.SkipIf(t, runtime.GOOS == "windows", "linux test")

natalieparellano · 2020-08-07T21:40:25Z

launch/launcher_test.go

+							h.SkipIf(t, runtime.GOOS == "windows", "linux test")
+						})
+
+						it("is script", func() {


I think I missed what it is that makes the process a script (it has args)... is that the default?

I actually don't see what makes this test case different from https://github.com/buildpacks/lifecycle/pull/370/files#diff-3841e5f60214a915376943a789c0747cR492

Hmmm the short when's might be confusing in the code but I wanted to make the go test output a bit more readable on the command line

Should hopefully tell you it is a script if it has no args and is linux:
LaunchProcess/Direct=false/buildpack-provided/buildpack_API_>=_0.4/has_no_args/linux/is_script

The link above didn't take me to a different test.

launch/process_test.go

sclevine · 2020-08-07T21:48:00Z

layers/factory.go

+	}
+	defer func() {
+		if closeErr := lw.Close(); err == nil {
+			err = closeErr


Needs named return param.

Signed-off-by: Emily Casey <ecasey@vmware.com>

* strip it out in the launch env Signed-off-by: Emily Casey <ecasey@vmware.com>

Signed-off-by: Emily Casey <ecasey@vmware.com>

* Adds acceptance tests * Refactors launcher Signed-off-by: Emily Casey <ecasey@vmware.com>

Signed-off-by: Emily Casey <ecasey@vmware.com>

Signed-off-by: Emily Casey <ecasey@vmware.com> Co-authored-by: Natalie Arellano <narellano@vmware.com>

* Set Platform API for build when creator is run Signed-off-by: Emily Casey <ecasey@vmware.com>

* Add and strip lifecycle dir from path * Don't use full paths in entrypoint in tests or warning text * Test cleanup Signed-off-by: Emily Casey <ecasey@vmware.com>

ekcasey requested review from sclevine, natalieparellano and yaelharel August 6, 2020 22:57

ekcasey added this to the lifecycle-0.9.0 milestone Aug 6, 2020

ekcasey added the api/platform/0.4 label Aug 6, 2020

ekcasey linked an issue Aug 6, 2020 that may be closed by this pull request

Multicall launcher #356

Closed

ekcasey mentioned this pull request Aug 7, 2020

Launcher Arguments POC #309

Closed

ekcasey force-pushed the multicall-launcher branch from 9a50bfb to c40f8e6 Compare August 7, 2020 18:19

ekcasey marked this pull request as ready for review August 7, 2020 18:20

ekcasey requested a review from a team as a code owner August 7, 2020 18:20

natalieparellano reviewed Aug 7, 2020

View reviewed changes

launch/process_test.go Outdated Show resolved Hide resolved

sclevine reviewed Aug 7, 2020

View reviewed changes

layers/factory.go

}

defer func() {

if closeErr := lw.Close(); err == nil {

err = closeErr

Copy link

Member

sclevine Aug 7, 2020

Choose a reason for hiding this comment

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

Needs named return param.

ekcasey and others added 12 commits August 7, 2020 19:19

Adds process type symlinks

2a88736

Signed-off-by: Emily Casey <ecasey@vmware.com>

Set ENTRYPOINT

cc49018

Signed-off-by: Emily Casey <ecasey@vmware.com>

Attempt to organize exporter

25c9ae8

Signed-off-by: Emily Casey <ecasey@vmware.com>

prepend process dir to path

294a93d

* strip it out in the launch env Signed-off-by: Emily Casey <ecasey@vmware.com>

fix windows

a08f3eb

Signed-off-by: Emily Casey <ecasey@vmware.com>

Fix rebase errors

095d036

Signed-off-by: Emily Casey <ecasey@vmware.com>

Multicall launcher

ac00503

* Adds acceptance tests * Refactors launcher Signed-off-by: Emily Casey <ecasey@vmware.com>

cleanup

8748dce

Signed-off-by: Emily Casey <ecasey@vmware.com>

Apply suggestions from code review

ec96286

Signed-off-by: Emily Casey <ecasey@vmware.com> Co-authored-by: Natalie Arellano <narellano@vmware.com>

Apply suggestions from code review

d5cfd75

Signed-off-by: Emily Casey <ecasey@vmware.com> Co-authored-by: Natalie Arellano <narellano@vmware.com>

more cleanup

3b7e7a3

* Set Platform API for build when creator is run Signed-off-by: Emily Casey <ecasey@vmware.com>

Review feedback

190dff9

* Add and strip lifecycle dir from path * Don't use full paths in entrypoint in tests or warning text * Test cleanup Signed-off-by: Emily Casey <ecasey@vmware.com>

ekcasey force-pushed the multicall-launcher branch 3 times, most recently from 47810b2 to 190dff9 Compare August 8, 2020 00:42

ekcasey merged commit 7617cba into main Aug 8, 2020

ekcasey deleted the multicall-launcher branch August 8, 2020 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multicall launcher #370

Multicall launcher #370

ekcasey commented Aug 6, 2020 •

edited

Loading

natalieparellano left a comment •

edited

Loading

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano left a comment

natalieparellano Aug 7, 2020 •

edited

Loading

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

natalieparellano Aug 7, 2020

ekcasey Aug 7, 2020

sclevine Aug 7, 2020

		@@ -61,12 +65,13 @@ func (cm *CacheMetadata) MetadataForBuildpack(id string) BuildpackLayersMetadata

		// NOTE: This struct MUST be kept in sync with `LayersMetadataCompat`

	h.SkipIf(t, runtime.GOOS == "windows", "windows test")
	h.SkipIf(t, runtime.GOOS == "windows", "linux test")

Multicall launcher #370

Multicall launcher #370

Conversation

ekcasey commented Aug 6, 2020 • edited Loading

natalieparellano left a comment • 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

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

natalieparellano left a comment

Choose a reason for hiding this comment

natalieparellano Aug 7, 2020 • 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

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

ekcasey commented Aug 6, 2020 •

edited

Loading

natalieparellano left a comment •

edited

Loading

natalieparellano Aug 7, 2020 •

edited

Loading