Include buildpack RFC0013 in specification #68
Conversation
There was a problem hiding this comment.
Noticed at comment in the RFC:
The entry point of this data into the lifecycle. We expect the data to be provided by the platform, and for the lifecycle to write it verbatim into the manifest without need to alter or otherwise manipulate the provided data.
This interaction should be spec'd out in the Platform API IMO. Otherwise how this data is propagated could make platforms and lifecycle implementation incompatible.
|
Should we consider changing the key to |
|
@jkutner Maybe we should create a single @iainsproat This is an interface between the platform and the lifecycle (as the platform provides source info to the lifecycle), and therefore should go in platform.md. |
|
@jkutner If folks are happy to change from @sclevine We had initially thought about making this PR to the platform / lifecycle interface spec but it didn't seem to be currently documenting anything around the export or output on the OCI image which is why we updated the buildpack spec instead. If we were to PR to the platform spec, would you propose that we need to add a new section with our relevant details? |
There was a problem hiding this comment.
My major concern is, in some ways, a critique of the existing platform spec, rather than a critique of how this specific feature is specified. Several lifecycle phases and their flags were previously unspecified. Currently the exporter adds several labels to the generated image that are also unspecified. Adding just this one flag and one label to the specification seems misleading when others are omitted.
I think we should not add this change to the spec at all, until it can be added as part of a major overall of the platform spec. I am okay adding this feature to lifecycle w/o adding it to the spec b/c it is analogous to several similar unspecified lifecycle features.
| @@ -89,6 +92,18 @@ Where: | |||
| - `-group` MUST specify input from a `group.toml` file path as defined in the [Data Format](#data-format) section. | |||
| - `-plan` MUST specify input from a Build Plan as defined in the [Buildpack Interface Specification](buildpack.md). | |||
|
|
|||
| #### Export phase | |||
There was a problem hiding this comment.
Adding the Export phase and it's usage here without also adding the Analyze and Restore phases to the spec seems more confusing to me than omitting all three.
There was a problem hiding this comment.
Isn't that a bit out of scope of their proposed change, though?
There was a problem hiding this comment.
Ah I see, (at least I think I'm understanding): you're suggesting we don't add it to the spec proper, but instead include the feature in the lifecycle, but leave the exporter out of the spec until we add all three missing phases and generally overhaul the lifecycle in the spec?
There was a problem hiding this comment.
I am happy to own updating the platform spec, it's long overdue. Afterward we can accept incremental changes.
Given that this is a specification rather than documentation adding Exporter with just this one flag is more confusing than not having it at all. By choosing to document a single flag we incorrectly imply something about its importance.
There was a problem hiding this comment.
Is CNB_PLATFORM_DIR part of the spec though if we're reserving that keyword?
There was a problem hiding this comment.
@ekcasey - we agree it would be preferable to have complete documentation. However, in the absence of complete documentation, we'd prefer partial documentation which covers the intent of RFC0013.
For our immediate purposes, we'd be happy to have the implementation merged and then we can merge this PR at a later point once the rest of the specification documentation is complete.
- Add app source metadata format - [RFC0013](https://github.com/buildpack/rfcs/blob/master/text/0013-app-layer-metadata-source.md) [#170205764] Signed-off-by: Shane Huston <shuston@pivotal.io> Signed-off-by: Velda Conaty <vconaty@pivotal.io>
* Changes will be moved to the platform specification document This reverts commit 3dc1b44. Signed-off-by: Velda Conaty <vconaty@pivotal.io>
* Moves the specification from Buildpack document to Platform document * Adds a new optional `-platform` flag to the lifecycle builder * Adds a new project.toml input file * Includes source, source.version, and source.metadata properties to project.toml * Provides a new section to specify the Config labels to be added to the OCI image * Includes a data type for the "io.buildpacks.project" Config label. [#170205764] Signed-off-by: Velda Conaty <vconaty@pivotal.io>
[#170205764] Signed-off-by: Velda Conaty <vconaty@pivotal.io>
* renamed flag to source-metadata * created an Export Phase section * removed redundant quotation marks from label key * renamed TOML file to source-metadata.toml [#170205764] Signed-off-by: Velda Conaty <vconaty@pivotal.io>
* follows comments received on PR ** remove whitespace ** add section to Table of Contents ** Renames `source-metadata` to `project-metadata` [#170205764] Signed-off-by: Carlo Colombo <ccolombo@pivotal.io> Signed-off-by: Velda Conaty <vconaty@pivotal.io>
- Renames OCI image to Exported image [#170205764] Signed-off-by: Velda Conaty <vconaty@pivotal.io>
|
@iainsproat l vote for merging after we specify the missing phases |
| ## Exported Image | ||
| The Exported image containing the compiled application is generated by the Build Image during the [Export Phase](#export-phase). | ||
|
|
||
| If the `/cnb/lifecycle/exporter` is provided with a `project-metadata.toml` file in the Platform directory, the lifecycle MUST add a `io.buildpacks.project` image config label to the Exported image. This label value is encoded JSON format of the `project-metadata.toml` contents, as described in the [data-format](#data-format) section. |
There was a problem hiding this comment.
@ekcasey any thoughts about using io.buildpacks.project.metadata instead of io.buildpacks.project to match the other labels? I noticed this key was already added to the lifecycle.
There was a problem hiding this comment.
@sclevine that suggestion makes sense to me
There was a problem hiding this comment.
here ya go - buildpacks/lifecycle#257
* Derived from PR #68 * Paths and label names have been changes to match the concensus arrived at during implementation - buildpacks/lifecycle#230 - buildpacks/lifecycle#257 Signed-off-by: Emily Casey <ecasey@pivotal.io>
|
Closing in favor of #81 |
* Derived from PR #68 * Paths and label names have been changes to match the concensus arrived at during implementation - buildpacks/lifecycle#230 - buildpacks/lifecycle#257 Signed-off-by: Emily Casey <ecasey@pivotal.io>
* Derived from PR #68 * Paths and label names have been changes to match the concensus arrived at during implementation - buildpacks/lifecycle#230 - buildpacks/lifecycle#257 Signed-off-by: Emily Casey <ecasey@pivotal.io>
[#170205764]
Signed-off-by: Shane Huston shuston@pivotal.io