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

Assume web is the default for older buildpacks #204

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

natalieparellano
Copy link
Member

@natalieparellano natalieparellano commented Mar 10, 2021

Fixes #160

Signed-off-by: Natalie Arellano narellano@vmware.com

@natalieparellano natalieparellano requested a review from a team as a code owner March 10, 2021 17:22
@natalieparellano natalieparellano marked this pull request as draft March 10, 2021 17:22
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member Author

@yaelharel I took a stab at it - do you mind taking a look at it? Note that these are only the platform changes...

Signed-off-by: Natalie Arellano <narellano@vmware.com>
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.

Looks good to me! @dwillist, would you mind reviewing it as well?

Signed-off-by: Natalie Arellano <narellano@vmware.com>
@ekcasey ekcasey added this to the Platform 0.6 milestone Mar 11, 2021
@ekcasey ekcasey marked this pull request as ready for review March 11, 2021 18:17
@@ -893,6 +899,8 @@ Where:

#### `metadata.toml` (TOML)
```toml
buildpack-default-process-type = "<process type>"
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 the "buildpack-" prefix here? Should we add a top-level key?

Copy link
Member Author

@natalieparellano natalieparellano Mar 17, 2021

Choose a reason for hiding this comment

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

We wanted buildpack- to differentiate between the buildpack-provided default and user-provided default (via the -process-type flag). We thought it would be confusing if the app had default process type some-user-type and metadata.toml contained default-process-type = some-buildpack-type. An alternative that we considered was deleting this key from metadata.toml prior to export, so that it is only used to pass information between phases.

As for whether it should be a top-level key, I'm not sure... we thought about adding default = <true or false> under each buildpack but then we also thought it would be confusing given that multiple buildpacks can each contribute a default and it wouldn't be obvious to the reader of this file that "last wins".

cc @yaelharel please add if I missed anything!

@nebhale nebhale requested a review from a team March 17, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants