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 an RFC about builder config #241

Closed
wants to merge 2 commits into from

Conversation

samj1912
Copy link
Contributor

@samj1912 samj1912 commented Aug 28, 2022

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Summary

This RFC proposes an easy way to configure builders to allow specifying a config.toml file that allows updating the Buildpack detect and build environment based on the configuration file.

Readable

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@samj1912 samj1912 requested a review from a team as a code owner August 28, 2022 12:38
@samj1912 samj1912 force-pushed the builder-config branch 2 times, most recently from 7339cc9 to 2855f50 Compare August 28, 2022 13:57
@loewenstein
Copy link

I am wondering about two things.
First, I am very skeptical about a precedence order that does not prioritize direct user input
Second, and maybe even more important, shouldn't this be proposed via CNB spec?

@samj1912
Copy link
Contributor Author

samj1912 commented Aug 28, 2022

I am wondering about two things.

First, I am very skeptical about a precedence order that does not prioritize direct user input

It gives builder/operators a choice which they currently don't have very easily. If they wish to prioritize user input they can use default (which is also the mode that is selected if the user doesn't specify a mode). If they wish to override it, they can use override. The fact that builder/buildpacks cannot control user input can be a major security concern. IMHO It should be entirely upto the buildpack and the builder operator to enforce this. This is no different than a buildpack using clear-env flag to omit using user provided platform env vars or a buildpack choosing to override/ignore platform env vars.

Second, and maybe even more important, shouldn't this be proposed via CNB spec?

It can be. CNB changes take a while to trickle down/be implemented. This is also something that the lifecycle currently allows buildpacks to achieve with the existing API,so I don't see why it can't be implemented at a paketo level if we can get immediate benefits from it.

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

Created buildpacks/rfcs#230 in case there is appetite for it upstream.

Co-authored-by: Daniel Mikusa <dan@mikusa.com>
@dmikusa
Copy link
Contributor

dmikusa commented Aug 29, 2022

Second, and maybe even more important, shouldn't this be proposed via CNB spec?

It can be. CNB changes take a while to trickle down/be implemented. This is also something that the lifecycle currently allows buildpacks to achieve with the existing API,so I don't see why it can't be implemented at a paketo level if we can get immediate benefits from it.

This is my concern as well. Should we do this at the Paketo level, or wait for it to be done in the lifecycle? I'm not opposed to implementing it now in Paketo if we can do it in such a way that we could later back this out of Paketo and just let the lifecycle handle it. If we can't, my fear is that we could end up stuck with something we have to support in Paketo that works one way and then later have a very similar, but not the same bit of functionality in lifecycle, which is very confusing for users.

@davidmirror-ops
Copy link

WG 2022-08-30 Notes:
[JL] should we push it upstream?
[SK] Doesn't truly matter where it's implemented: lifecycle or Paketo
Cautions about security (secrets/bindings) and pack

@davidmirror-ops
Copy link

WG 2022-09-06 Notes: there seems to be response from the CNB project, keeping an eye on to see if this RFC is needed here

@fg-j
Copy link

fg-j commented Sep 13, 2022

WG 2022-09-13 Notes:
Given that this idea is gaining traction upstream, Paketo is reluctant to implement at the buildpack level. This RFC is on hold until the CNB project's intentions around this proposal become more clear.

@davidmirror-ops
Copy link

WG 2022-09-20 Notes: no updates

@ryanmoran
Copy link
Member

WG 2022-09-27 Notes:
@samj1912 can this be moved back into a draft state since the upstream RFC seems to be making some progress?

@samj1912 samj1912 marked this pull request as draft September 28, 2022 00:01
@samj1912
Copy link
Contributor Author

Upstream rfc accepted.

@samj1912 samj1912 closed this Nov 20, 2022
@samj1912 samj1912 deleted the builder-config branch November 20, 2022 20:45
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.

None yet

6 participants