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.

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

Platform spec rewrite #87

Merged
merged 25 commits into from
Jul 8, 2020
Merged

Platform spec rewrite #87

merged 25 commits into from
Jul 8, 2020

Conversation

ekcasey
Copy link
Member

@ekcasey ekcasey commented Jun 10, 2020

When making decisions about whether or not to specify existing behavior, I defaulted to over-specifying, on the assumption that it would be easier to see the full breadth of what could be specified and remove things.

Copy link
Contributor

@nebhale nebhale left a comment

Choose a reason for hiding this comment

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

And of course, I'm totally unqualified to review the content of this PR as my familiarity with platforms is nearly zero.

platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
@ekcasey ekcasey added this to the Platform 0.3 milestone Jun 24, 2020
@ekcasey ekcasey linked an issue Jun 24, 2020 that may be closed by this pull request
@ekcasey ekcasey linked an issue Jun 24, 2020 that may be closed by this pull request
@nebhale nebhale marked this pull request as ready for review June 24, 2020 18:07
@nebhale nebhale requested a review from a team as a code owner June 24, 2020 18:07
platform.md Show resolved Hide resolved
platform.md Outdated

#### Rebase
Run image rebasing allows for fast stack updates for already-exported OCI images without require a rebuild. A rebase requires minimal data transfer when those images are stored on a Docker registry.
When a new stack version with the same stack ID is available, the app layers and launch layers SHOULD be rebased on the new run image by updating the image's configuration to point at the new run image.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to be more explicit than "rebased on the new run image"? Maybe something like:

create a new image using the app layers and launch layer on the new run image ...

Also, I'm not great at spec-speak, but maybe this should be "MUST" instead of "SHOULD"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to be more explicit than "rebased on the new run image"?

I took another stab at it. Let me know what you think.

Also, I'm not great at spec-speak, but maybe this should be "MUST" instead of "SHOULD"?

I think this is properly a SHOULD. It's up to the platform to kick off a rebase when a new run image is available. Platforms like pack can't do this automatically every time a new stack image is released. Therefore it is guidance to platform and/or end users rather than a requirement.

platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
ekcasey and others added 10 commits June 26, 2020 11:24
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
* Adds details to data format
* Alphabetizes

Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Co-authored-by: Ben Hale <bhale@vmware.com>
Co-authored-by: Joe Kutner <jpkutner@gmail.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
…-rewrite

Signed-off-by: Emily Casey <ecasey@vmware.com>
* Use bullet points to break up hard to read paragraphs
* Consistent use of - vs *
* Consistent capitalization of IF
* Consistent captialization of bullet point lines

Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
* Removes duplicated compatibility rules
* Clarifies language
* Other minor cleanup

Signed-off-by: Emily Casey <ecasey@vmware.com>
@nebhale nebhale requested a review from a team June 29, 2020 14:01
Copy link

@yaelharel yaelharel left a comment

Choose a reason for hiding this comment

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

@ekcasey, thank you for adding a lot of information. It's great!!!

buildpack.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved

## Stacks
A **stack** is a contract defined by a **build image** and a **run image** that are only updated with security patches.

Choose a reason for hiding this comment

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

Maybe it's worth adding an explanation on why we combine a build image and a run image under the term stack and why we update it only in security patches.
I'm not sure what is the right place to add it.
Maybe if you move the stack definition to be after the build image and the run image definitions, you can add it there.

Copy link
Member

Choose a reason for hiding this comment

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

Separate note: the contract isn't defined by the images, but by the functionality that the images provide.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe if we don't define stack by itself? Something like:

Suggested change
A **stack** is a contract defined by a **build image** and a **run image** that are only updated with security patches.
A **stack contract** is defined by a set of unchanging ABIs, command line interfaces, and functional behaviors. Images marked with a **stack ID** MUST implement the same contract.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't realize this line was moved and not changed. We shouldn't block merging on fixing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went to add a version of this suggestion with an accompanying definition of stack ID but I wasn't convinced it was easier to understand.

Given that we already have this definition in the existing spec maybe we can punt this improvement (and a possible reordering of terminology) to a future PR. It might be easier to think about these types of incremental improvements after we get this monster merged.

Copy link
Member

Choose a reason for hiding this comment

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

Should we create an issue to track this work?


A **run image** is an OCI image that provides the base from which **app images** are built

The **build environment** refers to the containerized environment in which the lifecycle executes buildpacks.

Choose a reason for hiding this comment

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

Following my comment about the stack definition, I think it's better (if possible) to use a term after defining it.
Therefore I would move the build environment definition to be before the build image definition, and the app images definition to be before the run image definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.... It gets tricky b/c app image references run image and run image references app image?

I don't want it to be confusing but some of these terms exist only in relationship to each other? Maybe we should just alphabetize them?

Choose a reason for hiding this comment

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

I know that it isn't possible in every term but I think that if it's possible, it's worth doing so.
If you decide that it's not possible, I think it's better to leave it as is and not alphabetize them (at least now, I expect to read the definition of the term that I've just see in the previous definition).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try to reorganize a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

How about we do this in a self-contained PR after this merges?

Choose a reason for hiding this comment

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

Good idea!

platform.md Outdated
The **launcher** refers to a lifecycle executable packaged in the app image for the purpose of executing processes at runtime

#### Additional Terminology
An **image reference** refers to either a **tag reference** or **digest reference**

Choose a reason for hiding this comment

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

See the other comments about the order of the definitions (of course this is how I prefer to read it but maybe others have different opinion about it :) ).

buildpack.md Outdated Show resolved Hide resolved
- IF `<skip-restore>` is `true` the `creator` SHALL skip layer analysis and skip the entire Restore phase.
- IF the platform provides one or more `<tag>` inputs they SHALL be treated as additional `<image>` inputs to the `exporter`

Outputs produced by `creator` are identical to those produced by `exporter`.

Choose a reason for hiding this comment

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

Maybe it's worth adding why a user would want to use the creator over the 5 phases separately.

Copy link
Member

@hone hone Jul 8, 2020

Choose a reason for hiding this comment

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

Is this just the difference b/t spec and documentation?

platform.md Outdated Show resolved Hide resolved
Cache locality and availability MAY vary between platforms.

## Data Format

### order.toml (TOML)
### Files

Choose a reason for hiding this comment

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

Maybe it's worth adding an example of each file (or pointing to a location in the samples repository with such examples).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should provide examples outside the spec in more friendly formats like (future) lifecycle docs.

Choose a reason for hiding this comment

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

Sounds good but don't you think it's worth adding a link in the spec to these examples?
Anyway, if these examples don't exist yet, we can discuss it when it'll be relevant :)

@@ -7,64 +7,114 @@ A platform orchestrates a lifecycle to make buildpack functionality available to
Examples of a platform might include:

1. A local CLI tool that uses buildpacks to create OCI images
2. A plugin for a continuous integration service that uses buildpacks to create OCI images
3. A cloud application platform that uses buildpacks to build source code before deployment
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep the numbering explicit, so that it's easier to find sections without a markdown rendering tool For example, if someone says "see section 1, item 4b," it's difficult to find that item without rendering the document. Not having to render the document to read it is generally why I prefer markdown to other markups. That said, I'm okay with this change if others disagree with me (and if we make it consistently).

(On that note, I've noticed that many specifications avoid unordered (e.g., bulleted) lists entirely, because it makes referring to individual items difficult. Would be great if we could pick a consistent rule for lists like "always use 1. -> a. -> i. and never use explicit numbering")

Copy link
Contributor

Choose a reason for hiding this comment

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

This numbering thing is a problem that I don’t think we have a good handle on yet. Including the numbers explicitly requires users to always remember to update them whenever a change is made (and even the best can forget updates). Also using ordered lists has a tendency to break links as data is inserted and remove (note the anchor names in the TOC).

My personal preferences run towards ordered lists for the same reasons you like them, but the downsides of using them are so high that I can’t justify it 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

if someone says "see section 1, item 4b," it's difficult to find that item without rendering the document

How likely are you to lookup "section 1, item 4b" in the unrendered version though? Given that we are a bit inconsistent right now I wonder if we could bring decide on guidelines and bring all the specs into compliance with them separately from this PR.

platform.md Outdated Show resolved Hide resolved

The following is a non-exhaustive list of terms defined in the [OCI Image Format Specification](https://github.com/opencontainers/image-spec) used throughout this document:
* **image config** https://github.com/opencontainers/image-spec/blob/master/config.md#oci-image-configuration
* **imageID** - https://github.com/opencontainers/image-spec/blob/master/config.md#imageid
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* **imageID** - https://github.com/opencontainers/image-spec/blob/master/config.md#imageid
* **ImageID** - https://github.com/opencontainers/image-spec/blob/master/config.md#imageid

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to go with consistent lower-casing for imageID and diffID

platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
ekcasey and others added 2 commits July 7, 2020 17:57
Co-authored-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
* don't capitalize non RFC 2119 words
* small gramatiical fixes
* improved launcher process selection

Signed-off-by: Emily Casey <ecasey@vmware.com>
@ekcasey ekcasey force-pushed the platform-spec-rewrite branch 2 times, most recently from 1085089 to c7e09cc Compare July 7, 2020 22:20
Signed-off-by: Emily Casey <ecasey@vmware.com>
A hack so we can use illegal comments and still get syntax highlighting.

Signed-off-by: Emily Casey <ecasey@vmware.com>
* Fixes inaccurate commandline usage

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

@hone hone left a comment

Choose a reason for hiding this comment

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

I don't have any blocking comments, but I think we should make sure to capture the issues on this repo for the things we're punting on.

@nebhale nebhale merged commit 944ff5f into platform/0.3 Jul 8, 2020
@nebhale nebhale deleted the platform-spec-rewrite branch July 8, 2020 18:23
@nebhale nebhale mentioned this pull request Jul 8, 2020
ekcasey added a commit that referenced this pull request Oct 16, 2020
Previously stack ID requirements including:
* allowed characters
* global uniqueness
were only specified in the buildpack spec, in the buildpack.toml data formt section. This PR moves stack ID description and requirements in the Platform spec with the rest of the stack concepts.

Clarifies stack concepts, incorporating feedback that surfaced in previous PR reviews #87 (comment).

Signed-off-by: Emily Casey <ecasey@vmware.com>
@ekcasey ekcasey mentioned this pull request Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CNB_PLATFORM_API should configure lifecycle platform API Document Platform API 0.3
7 participants