Default process #520

yaelharel · 2021-02-01T18:49:27Z

Add support for Builpack API 0.6
Add support of default process type to buildpacks

Fixes #457
Fixes #458

cc @dwillist

buildpacktoml.go

builder_test.go

buildpacktoml_test.go

builder_test.go

builder.go

exporter.go

exporter_test.go

natalieparellano

@yaelharel @dwillist it's great to see this, nice work wrangling all of the complexity of this feature :)

I think we want to preserve the existing "choose the only process as default" behavior for platform api 0.4 and 0.5.

Beyond that I think the refactor I suggested (move platform api logic into builder.go) should make this easier to reason about and test - what do you think?

yaelharel · 2021-02-02T05:20:09Z

@natalieparellano, thank you for your feedback!
Please see the changes I've made and the comments I left to some of your ideas.

builder.go

builder_test.go

buildpacktoml.go

builder.go

buildpacktoml.go

buildpacktoml_test.go

exporter.go

buildpacktoml_test.go

Signed-off-by: Yael Harel <yharel@vmware.com>

Buildpacks that implements api < 0.6 should have the default value set to false Signed-off-by: Yael Harel <yharel@vmware.com>

no matter if they are having the same type or different types Signed-off-by: Yael Harel <yharel@vmware.com>

builder.go

yaelharel · 2021-02-05T02:25:05Z

@jabrown85 @natalieparellano @ekcasey, we're going to run some manual tests tomorrow, but we've made all of the changes that we talked about so I think it's good time for another review.
Thank you!

cc @dwillist

- Introduce a new field in metadata.toml - Buildpack-default-process-type - Remove the default field from each process in metadata.toml - Sort the processes in metadata.toml alphabetically based on their types - Don't fall back to old default process if another default buildpack overrided it and then the new default process was overrided by a non-default process (X default -> Y default -> Y not-default ==> no default process) Signed-off-by: Daniel Thornton <dwillist@vmware.com> Signed-off-by: Yael Harel <yharel@vmware.com> Signed-off-by: Daniel Thornton <dwillist@vmware.com>

exporter_test.go

launch/launch.go

builder.go

metadata.go

builder_test.go

exporter_test.go

natalieparellano · 2021-02-05T15:50:25Z

@yaelharel @dwillist this is looking great! I left couple suggestions, mostly cosmetic. I think we'll want to add the omitempty to json:"buildpack-default-process-type" to avoid changing the io.buildpacks.build.metadata label, but overall this seems to be just about ready.

Signed-off-by: Daniel Thornton <dwillist@vmware.com> Signed-off-by: Natalie Arellano <narellano@vmware.com> Signed-off-by: Yael Harel <yharel@vmware.com>

natalieparellano · 2021-02-05T17:48:22Z

@jabrown85 did you want to have another look before we merge?

yaelharel · 2021-02-05T17:51:43Z

We did a few more changes thanks to @natalieparellano feedback!

We also ran the following tests manually (see #520 (comment) for the exact commands):

Build a lifecycle image based on our code
Build a pack with the changes from Support Platform APIs 0.5, 0.6 pack#1051
Create a builder with the new lifecycle image
Run some scenarios while passing the --buildpack flag in the right order and test the expected results:
- Print of the default process
- Warning in case a buildpack overrides the default process
- Metadata content by getting into the app image and checking the config/metadata.toml file (default=false and buildpack-default-process-type=false isn't printed to the file)

The scenarios that we tested:

Old buildpack with a web process
⇒ default = web
New buildpack with a default worker process
⇒ default = worker
Old buildpack with no web process
⇒ no default
New buildpack with a default worker process → new buildpack with a default another-worker process
⇒ default = another-worker
New buildpack with a default worker process → old builpdack with a web process
⇒ default = web
New buildpack with a default worker process → new buildpack with a non-default worker process
⇒ no default + override warning
New buildpack with a default Y process → new buildpack with a default X process → new buildpack with a non-default Y process
⇒ default = X (no warning)

yaelharel requested review from jabrown85, ekcasey and natalieparellano February 1, 2021 18:49

yaelharel requested a review from a team as a code owner February 1, 2021 18:49