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

Add support for platform API 0.7 and 0.8 #889

Merged
merged 5 commits into from
Nov 24, 2021

Conversation

sambhav
Copy link
Contributor

@sambhav sambhav commented Nov 19, 2021

This PR adds support for 0.7 and 0.8.

Resolves #886

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@sambhav sambhav mentioned this pull request Nov 19, 2021
@sambhav sambhav force-pushed the platform-api branch 2 times, most recently from fcb833b to efc2914 Compare November 19, 2021 01:09
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2021

Codecov Report

Merging #889 (8907075) into main (5c07ecf) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
+ Coverage   68.08%   68.25%   +0.17%     
==========================================
  Files         116      116              
  Lines        5164     5192      +28     
==========================================
+ Hits         3516     3544      +28     
  Misses       1287     1287              
  Partials      361      361              
Impacted Files Coverage Δ
pkg/cnb/builder_builder.go 84.07% <ø> (ø)
pkg/apis/build/v1alpha2/build_pod.go 98.19% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c07ecf...8907075. Read the comment docs.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@matthewmcnew matthewmcnew self-requested a review November 19, 2021 19:28
@tomkennedy513 tomkennedy513 self-requested a review November 19, 2021 21:06
@dumez-k dumez-k self-requested a review November 19, 2021 21:06
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@tylerphelan
Copy link
Contributor

do we want to include the stack.toml or -run-image for analyze? https://github.com/buildpacks/spec/blob/main/platform.md#inputs (@natalieparellano)

@sambhav
Copy link
Contributor Author

sambhav commented Nov 23, 2021

do we want to include the stack.toml or -run-image for analyze? buildpacks/spec@main/platform.md#inputs (@natalieparellano)

It's not needed for the way kpack builds things. It can just use the defaults (run image from stack.toml and stack.toml location as /cnb/stack.toml). We also never passed -stack in export before.

@pviraj59 pviraj59 requested a review from tylerphelan November 23, 2021 15:39
@matthewmcnew
Copy link
Collaborator

Do we want to also relax our mixin validation logic within that platform api?

@sambhav sambhav requested a review from matthewmcnew November 24, 2021 08:36
@matthewmcnew
Copy link
Collaborator

Added an additional issue to track mixin validation relaxation: #891

},
},
ifWindows(buildContext.os(), addNetworkWaitLauncherVolume(), useNetworkWaitLauncher(dnsProbeHost))...,
func() corev1.Container {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to explore moving the instantiation of these containers within the pod spec (with the accepted possibility of some duplication). The step pattern was originally added to allow all the containers to be explicitly provided in the pod spec with some logic.

I think we can have something like:

if platformAPILessThan07 {
    step(detect...)
    step(analyze..)
}else {
    step(analyze...)
    step(detect..)
}
						

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to explore that in a future refactor though :)

@tylerphelan tylerphelan merged commit ffd8cd1 into buildpacks-community:main Nov 24, 2021
@sambhav sambhav deleted the platform-api branch November 24, 2021 22:55
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.

Support Platform api 0.7 / platform api 0.8
5 participants