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

Adds platform path to build and detect context #134

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Conversation

ryanmoran
Copy link
Member

@ryanmoran ryanmoran commented Mar 10, 2021

Summary

The location of the platform metadata is given to the build and detect processes when they are invoked. This location includes details about user-provided environment variables and other platform extensions that we might want buildpack authors to take advantage of. This addition includes that filesystem location as a parameter on both DetectContext and BuildContext.

In the process of including this new context, I found that the platform location was hardcoded in the existing postal.Service.Install implementation. I've included a new postal.Service.Deliver method that allows for the platform path to be given as an argument. The existing Install method will reuse the Deliver method with a hardcoded platform path to maintain backwards compatibility. The Install method has also been marked as deprecated.

Additionally, as I was including a new type that spans both Detect and Build, it made sense for that type to live in its own file. Once I did that, I thought maybe all the types that aren't directly used in the immediate Detect or Build functions ought to have their own files to make them a bit easier to find. So, this also includes a bunch of new files that contain those types that have been moved out of detect.go and build.go into their own files.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have reviewed the styleguide for guidance on my code quality.

@sophiewigmore
Copy link
Member

Could you explain how having Platform as a parameter on the contexts is a benefit? Is it so that we can modify platform fields during detect and build? Why would we need to do this?

// validated against the checksum value provided on the Dependency and will
// error if there are inconsistencies in the fetched result.
func (s Service) Deliver(dependency Dependency, cnbPath, layerPath, platformPath string) error {
dependencyMappingURI, err := s.mappingResolver.FindDependencyMapping(dependency.SHA256, filepath.Join(platformPath, "bindings"))
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that the platform path could be anything other than /platform, but I need see in the spec that that's just a default value. Random question - at what point could the platform path be set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arguably a platform that isn't pack or kpack could set a different filepath as the Platform path when it invokes the lifecycle and then hardcoding this like we have wouldn't work.

@ryanmoran
Copy link
Member Author

Could you explain how having Platform as a parameter on the contexts is a benefit? Is it so that we can modify platform fields during detect and build? Why would we need to do this?

The CNB spec includes a few extensions that expose information to buildpacks via the Platform path. Having access to that path in a buildpack would be useful for those buildpacks to integrate with those extensions if they needed to.

@sophiewigmore
Copy link
Member

The CNB spec includes a few extensions that expose information to buildpacks via the Platform path. Having access to that path in a buildpack would be useful for those buildpacks to integrate with those extensions if they needed to.

Could you point me to an example in the spec, please?

@ryanmoran
Copy link
Member Author

The Bindings Extension spec is a good example of this: https://github.com/buildpacks/spec/blob/main/extensions/bindings.md.

- moves types referenced in build and detect contexts into their own
files to make it a bit easier to find them when navigating the repo
- adds new postal.Service.Deliver method that allows the user to pass in
a platform path location. The existing postal.Service.Install method
reuses Deliver with a hardcoded platform path of /platform and is marked
as deprecated
sophiewigmore
sophiewigmore previously approved these changes Mar 15, 2021
@sophiewigmore sophiewigmore merged commit dfcde18 into main Mar 17, 2021
@sophiewigmore sophiewigmore deleted the platform-path branch March 17, 2021 21:14
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.

2 participants