Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Extension Spec for Builder #116

Merged
merged 14 commits into from
Nov 13, 2020
Merged

Add Extension Spec for Builder #116

merged 14 commits into from
Nov 13, 2020

Conversation

dfreilich
Copy link
Member

@dfreilich dfreilich commented Sep 15, 2020 •
edited by ekcasey

@dfreilich
Add initial specification for Builder
4bcb5c2 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich
Add layer information to builder RFC
08b7a1f 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich requested a review from a team as a code owner September 15, 2020 19:54
dfreilich
dfreilich commented Sep 15, 2020
View reviewed changes
text/0000-builder-spec.md Outdated Show resolved Hide resolved
@dfreilich
Link json objects to their definitions
31c6185 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich changed the title Extension spec builder Add Builder to Distribution Spec Sep 16, 2020
ekcasey
ekcasey reviewed Sep 16, 2020
View reviewed changes
Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

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

In my mind the big missing piece here is a API number or similar concept for builder images. An API number would allow us to change label schemas and file paths in an explicit and predictable way.

text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Show resolved Hide resolved
@dfreilich
Update description of builder and lifecycle
41b9a4a 
* Add reference to lifecycle descriptor file

Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich
Copy link
Member Author

Feedback from WG meeting (09/16):

  • It should be in extension spec, because platforms don't need to use builders at all. Add a link in platform spec noting that a builder is a fully functioning build-environment of a stack. kpack, despite having different style builders, still tries to support the same spec of builders.
  • Builder API should certainly be versioned. There was a suggestion for using OCI media-type labels to define the version on the builder image, with a cross-cutting concern that we wouldn't want to lose support for any registries just because we use an OCI compliant feature that isn't necessarily supported on every registry.

I'll rework it using that feedback.

@dfreilich dfreilich changed the title Add Builder to Distribution Spec Add Extension Spec for Builder Sep 17, 2020
@dfreilich
Change to Extension spec
6997a59 
* Add concept of versioning

Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich
Clarify why not versioning with mediaType
ef7d7e5 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich mentioned this pull request Sep 17, 2020
Merged
@nebhale nebhale requested a review from a team September 21, 2020 20:26
@hone
Copy link
Member

hone commented Sep 23, 2020

Is a file like builder.toml pack specific or something included here?

@dfreilich
Copy link
Member Author

@hone It came up here: #116 (comment). Consensus in WG was that it was pack specific.

ekcasey
ekcasey reviewed Sep 23, 2020
View reviewed changes
text/0000-builder-spec.md Show resolved Hide resolved
text/0000-builder-spec.md Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
@dfreilich
Incorporate @ekcasey 's suggestions
211b74c 
* Spelling/grammar corrections
* Move builder.api to its own Labels
* Add drawback
* Clarify spec changes

Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich
Copy link
Member Author

@buildpacks/core-team There were 2 open questions from @ekcasey I wanted to get feedback for:

  1. Should we specify a dedicated directory for service bindings, like /platform/bindings, and require builders to set $SERVICE_BINDING_ROOT to /platform/bindings?
  2. Do we actually want to specify any literal file paths or should we use CNB_X env vars to be set to an appropriate directory?

@dfreilich
Add CNB_APP_DIR and SERVICE_BINDING_ROOT to RFC
19f6bf0 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich
Incorporate WG Feedback:
2dbaebf 
* Use CNB_X vars to define directories and allow maximum flexibility
* Require SERVICE_BINDING_ROOT to be at CNB_PLATFORM_DIR/bindings

Signed-off-by: David Freilich <dfreilich@vmware.com>
ekcasey
ekcasey approved these changes Oct 21, 2020
View reviewed changes
text/0000-builder-spec.md Outdated Show resolved Hide resolved
text/0000-builder-spec.md Outdated Show resolved Hide resolved
@dfreilich
Incorporate changes from @ekcasey
9420714 
* Fix spelling
* Specify that the env_dirs must correspond to a dir in the builder

Signed-off-by: David Freilich <dfreilich@vmware.com>
jromero
jromero reviewed Oct 26, 2020
View reviewed changes
text/0000-builder-spec.md Outdated Show resolved Hide resolved
jromero
jromero reviewed Oct 26, 2020
View reviewed changes
text/0000-builder-spec.md Outdated Show resolved Hide resolved
@dfreilich
Incorporate @jromero 's suggestions
7b6f952 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@sclevine
Copy link
Member

FCP closing on 11/4

@ekcasey ekcasey mentioned this pull request Nov 10, 2020
Closed
@jromero jromero mentioned this pull request Nov 10, 2020
Open
2 tasks
ekcasey added a commit that referenced this pull request Nov 13, 2020
@ekcasey
RFC 0065
67f4362 
[#116]

Signed-off-by: Emily Casey <ecasey@vmware.com>
@ekcasey ekcasey merged commit d800213 into buildpacks:main Nov 13, 2020
@dfreilich dfreilich mentioned this pull request Feb 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jromero jromero jromero left review comments

@hone hone hone approved these changes

@nebhale nebhale nebhale approved these changes

@sclevine sclevine sclevine approved these changes

@ekcasey ekcasey ekcasey approved these changes

@natalieparellano natalieparellano natalieparellano approved these changes

@jkutner jkutner Awaiting requested review from jkutner

Assignees
No one assigned
Labels
issues-created/core issues-created/implementation issues-created/learning issues-created/platform scope/project spec/extensions/builder
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dfreilich @hone @nebhale @sclevine @jkutner @jromero @ekcasey @natalieparellano