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 project descriptor #690

Merged
merged 2 commits into from
May 25, 2021

Conversation

sambhav
Copy link
Contributor

@sambhav sambhav commented May 9, 2021

This PR adds support for the following attributes of a project descriptor -

  1. Build Env Variables
  2. Includes and excludes

The implementation of the project descriptor is pretty similar to the pack implementation.

  • The build env vars set by the project descriptor are overridden by the ones set by the user explicitly.
  • The includes and excludes use the go-gitignore library which is also used by pack and required by the spec (the spec says that these values are to be interpreted in the same way as gitignore)

Fixes #288

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2021

Codecov Report

Merging #690 (b60f465) into main (9259302) will increase coverage by 0.10%.
The diff coverage is 72.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   66.93%   67.03%   +0.10%     
==========================================
  Files          89       91       +2     
  Lines        3638     3680      +42     
==========================================
+ Hits         2435     2467      +32     
- Misses        853      858       +5     
- Partials      350      355       +5     
Impacted Files Coverage Δ
pkg/cnb/env_vars.go 55.55% <55.55%> (ø)
pkg/cnb/project_descriptor.go 72.97% <72.97%> (ø)
pkg/apis/build/v1alpha1/build_pod.go 97.65% <100.00%> (+0.01%) ⬆️
pkg/cnb/platform_env_vars_setup.go 60.00% <100.00%> (+10.00%) ⬆️

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 9259302...b60f465. Read the comment docs.

@sambhav sambhav force-pushed the project-descriptor branch 2 times, most recently from dcfcec2 to e689be8 Compare May 10, 2021 14:40
@matthewmcnew
Copy link
Collaborator

matthewmcnew commented May 11, 2021

Excellent work @samj1912!

I wanted to call out the some questions I had:

  1. How should we handle project descriptor specified build.buildpacks? Is that something you plan on using with kpack? Would it make sense to rewrite the order if the provided buildpacks are in the running builder?
  2. Should we do anything if a descriptor build.builder is specified? Perhaps log a warning to the user?
  3. Would it make sense to validate the license similar to what pack currently does?
  4. Similar to pack's --descriptor flag should we provide the ability for kpack users to specify the path to a descriptor on the image's spec? This might be something to target in the upcoming new api version.

@sambhav
Copy link
Contributor Author

sambhav commented May 11, 2021

Excellent work @samj1912!

I wanted to call out the some questions I had:

  1. How should we handle project descriptor specified build.buildpacks? Is that something you plan on using with kpack? Would it make sense to rewrite the order if the provided buildpacks are in the running builder?

I am not sure how to handle this. Currently pack creates an ephemeral builder with the specified buildpacks. As a cluster operator providing cluster builders, I may not want users to skip buildpacks intended for them. For example, let's say a cluster builder includes some buildpack that runs some security checks as a part of the build process. Users could possibly bypass this buildpack. The other issue is users handling buildpack version updates. Let's say a user specifies a certain buildpack and it's version and on the cluster builder side, this buildpack version is updated. Suddenly a completely valid app may stop building as it can't find the specified buildpack. Given that the spec says a platform should account for the buildpack keys but not must and given that kpack as a platform handles a longer term lifecycle of an application than something like pack, I'm tempted not to say that this might be too much of a support burden and may open up possible security issues on the operator side with users skipping important updates/buildpacks. I think what we can instead do is log a warning. But I'm open to suggestions on how to handle this.

  1. Should we do anything if a descriptor build.builder is specified? Perhaps log a warning to the user?

Makes sense to me. Again with the above caveat. Currently pack overrides the builder key in the project descriptor with what is explicitly provided by the user as a command line flag. The current implementation in this PR assumes anything provided by the user in the resource manifest overrides what is specified by the project descriptor (for example environment variables) . Given that the user already chose a builder to use, it makes sense that the builder in the resource manifest override the one in the project descriptor. A similar argument could be made for the above case of buildpacks.

  1. Would it make sense to validate the license similar to what pack currently does?

Possibly, we currently don't parse any project fields at all. But we can if we want to expose that to the project source label maybe?

  1. Similar to pack's --descriptor flag should we provide the ability for kpack users to specify the path to a descriptor on the image's spec? This might be something to target in the upcoming new api version.

Yup, makes sense to add support for this. I wanted to get something in without spec changes.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@tylerphelan
Copy link
Contributor

I think long term we should have a solution to the build.buildpacks use case but for now we could move forward with this! We just need make sure we document that build.buildpacks is not supported currently.

@tylerphelan tylerphelan self-requested a review May 13, 2021 19:14
@matthewmcnew
Copy link
Collaborator

To document the discussion from our May 24th, OSS WG:

Questions 1/2 (How to handle build.buildpacks build.builder?). It is unclear how we will handle build.buildpacks due to the needs of a kpack "operator" to ensure that kpack image's are using specific buildpacks and the need to prevent buildpacks that are not blessed by the operator (For example: a buildpack that does not provide accurate BOM metadata). This conversation will likely be complicated by the introduction of inline buildpacks that users would likely expect to work on kpack similarly to pack. In the meantime, we are going to just log an info message to the user if the supplied project.toml contains those keys.

Question 3 (Should we validate the project.toml license?). At this time due to the passive nature of scheduled kpack's builds we feel like that validating this unused field will lead to unnecessary build failures.

Question 4 (Should we support setting an alternative path to the descriptor?) Yes, we plan on providing this and it coinciding it with the release of the next api version.

@matthewmcnew matthewmcnew merged commit a100389 into buildpacks-community:main May 25, 2021
@sambhav sambhav deleted the project-descriptor branch May 26, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for project.toml
5 participants