Enhance build plan and expose env vars during detection #27

sclevine · 2018-11-05T02:19:38Z

Formalizes #24.

nebhale

Just some questions that might lead to some clarifications.

buildpack.md

nebhale · 2018-11-05T15:46:45Z

buildpack.md

-The lifecycle MUST NOT include any changes in this map that are output by optional buildpacks that returned non-zero exit statuses.
-The final Build Plan is the complete combined map that includes the output of the final `/bin/detect` executable.
+The lifecycle MUST NOT include any changes in this map that are contributed by optional buildpacks that returned non-zero exit statuses.
+The final Build Plan is the fully-merged merged map that includes the contributions of the final `/bin/detect` executable.


Is the definition of "fully-merged" ever defined anywhere? Is it some sort of deep merge, a last-wins shallow merge, or something else?

The last-wins shallow merge behavior is defined with:

The lifecycle MUST construct this map such that the top-level values from later buildpacks override the entire top-level values from earlier buildpacks.

nebhale · 2018-11-05T15:51:47Z

buildpack.md

@@ -639,7 +669,6 @@ Buildpacks MUST specify:
 ```toml
 [<dependency name>]
 version = "<dependency version>"
-provider = "<buildpack ID>"


nebhale · 2018-11-05T15:53:03Z

@jkutner and @hone If possible, I'd love for you to review this ASAP. At the moment our buildpack development is being delayed by the lack of environment variables at detect time and this would free us up to proceed.

jkutner · 2018-11-05T16:25:05Z

@sclevine is there a coupling between the build plan changes and the env vars for bin/detect? The latter seems far less intrusive, and I think we could merge that quickly to unblock @nebhale. The Build Plan changes are more to chew on.

jkutner · 2018-11-05T16:25:49Z

buildpack.md

@@ -70,19 +70,23 @@ The lifecycle MUST invoke these executables as described in the Phase sections.

 Executable: `/bin/detect`, Working Dir: `<app[AR]>`


I think we need to add <platform[AR]> here.

Actually <platform[A]> because of the build plan

Good catch! Fixed

jkutner · 2018-11-05T16:33:54Z

@nebhale can you give me an example use case for needing env vars during detect? Because we're talking about a new arg to /bin/detect I really want this think this through

nebhale · 2018-11-05T16:43:38Z

@jkutner In general, we use service bindings to declare intent for our integrations; e.g. existence of a new-relic service means you want the buildpack to put a New Relic javaagent on the command line and ensure that the licenseKey is extracted and set as a system property. However, there are a small handful of integrations that are not triggered by service bindings like Debug and JMX integrations. Today these are governed by setting a configuration value (i.e. cf set-env <APP> JBP_CONFIG_DEBUG '{ enabled : true }'). The CNB equivalent of that would be the existence of $BP_DEBUG and we cannot determine that at detect time today.

sclevine · 2018-11-05T16:45:38Z

Both the build plan changes and exposing the environment variables require adding the <platform> directory as an argument to /bin/detect.

The only breaking change is that /bin/detect has to write build plan additions to the <platform> directory. However, this normalizes the /bin/build and /bin/detect interfaces so that they both can log to stdout. This seems desirable, regardless of any other changes.

sclevine · 2018-11-05T17:29:36Z

I have a use case for user-provided env vars in /bin/detect as well. For the CF ruby buildpack, we look at BUNDLE_GEMFILE to determine whether your app is a ruby app with a non-standard Gemfile location.

- Normalize build/detect interfaces - Require Bash 3 for develop Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

jkutner

Thanks for working me through this.

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <platform>/plan, and updates Build to allow claims on build plan dependencies via <platform>/plan. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

nebhale

I'd like <plan> to move to $2 in build and develop for symmetry.

nebhale

I'd like <plan> to move to $2 in build and develop for symmetry.

nebhale · 2018-11-15T20:59:45Z

buildpack.md

@@ -110,7 +115,7 @@ Executable: `/bin/build <platform[AR]> <cache[EC]> <launch[EI]>`, Working Dir: `

 ### Development

-Executable: `/bin/develop <platform[A]> <cache[EC]>`, Working Dir: `<app[A]>`
+Executable: `/bin/develop <platform[AR]> <cache[EC]>`, Working Dir: `<app[A]>`


Missing <plan>

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

nebhale

Ship it.

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <plan>, and updates Build to allow claims on build plan dependencies via <plan>. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <platform>/plan, and updates Build to allow claims on build plan dependencies via <platform>/plan. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <plan>, and updates Build to allow claims on build plan dependencies via <plan>. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <platform>/plan, and updates Build to allow claims on build plan dependencies via <platform>/plan. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <plan>, and updates Build to allow claims on build plan dependencies via <plan>. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io>

This change updates the library to match upcoming spec changes. It adds the Platform to Detect, updates the Detect logger to use stdout, updates Detect's build plan output to write to <plan>, and updates Build to allow claims on build plan dependencies via <plan>. [buildpacks/spec#27] Signed-off-by: Ben Hale <bhale@pivotal.io> [ci skip] Incremental commit. Please do not base any external work off of this commit. Signed-off-by: Ben Hale <bhale@pivotal.io>

sclevine requested review from jkutner, hone and nebhale November 5, 2018 02:19

sclevine self-assigned this Nov 5, 2018

nebhale approved these changes Nov 5, 2018

View reviewed changes

jkutner reviewed Nov 5, 2018

View reviewed changes

Enhance build plan + expose env vars during detect

7abe622

- Normalize build/detect interfaces - Require Bash 3 for develop Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

sclevine force-pushed the 24-enhanced-build-plan branch from aa52424 to 7abe622 Compare November 6, 2018 04:06

Limit writes to <platform>/env/

dc5092a

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

jkutner approved these changes Nov 6, 2018

View reviewed changes

nebhale suggested changes Nov 15, 2018

View reviewed changes

Move plan argument to $2

f364f32

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>

nebhale approved these changes Nov 15, 2018

View reviewed changes

sclevine merged commit 9dc3139 into master Nov 20, 2018

nebhale deleted the 24-enhanced-build-plan branch August 30, 2019 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance build plan and expose env vars during detection #27

Enhance build plan and expose env vars during detection #27

sclevine commented Nov 5, 2018

nebhale left a comment

nebhale Nov 5, 2018

sclevine Nov 5, 2018

nebhale Nov 5, 2018

nebhale commented Nov 5, 2018

jkutner commented Nov 5, 2018

jkutner Nov 5, 2018

jkutner Nov 5, 2018

sclevine Nov 5, 2018

jkutner commented Nov 5, 2018

nebhale commented Nov 5, 2018

sclevine commented Nov 5, 2018

sclevine commented Nov 5, 2018

jkutner left a comment

nebhale left a comment

nebhale left a comment

nebhale Nov 15, 2018

nebhale left a comment

		@@ -70,19 +70,23 @@ The lifecycle MUST invoke these executables as described in the Phase sections.

		Executable: `/bin/detect`, Working Dir: `<app[AR]>`

Enhance build plan and expose env vars during detection #27

Enhance build plan and expose env vars during detection #27

Conversation

sclevine commented Nov 5, 2018

nebhale left a comment

Choose a reason for hiding this comment

nebhale Nov 5, 2018

Choose a reason for hiding this comment

sclevine Nov 5, 2018

Choose a reason for hiding this comment

nebhale Nov 5, 2018

Choose a reason for hiding this comment

nebhale commented Nov 5, 2018

jkutner commented Nov 5, 2018

jkutner Nov 5, 2018

Choose a reason for hiding this comment

jkutner Nov 5, 2018

Choose a reason for hiding this comment

sclevine Nov 5, 2018

Choose a reason for hiding this comment

jkutner commented Nov 5, 2018

nebhale commented Nov 5, 2018

sclevine commented Nov 5, 2018

sclevine commented Nov 5, 2018

jkutner left a comment

Choose a reason for hiding this comment

nebhale left a comment

Choose a reason for hiding this comment

nebhale left a comment

Choose a reason for hiding this comment

nebhale Nov 15, 2018

Choose a reason for hiding this comment

nebhale left a comment

Choose a reason for hiding this comment