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

RFC: Support for pack.toml #189

Closed
wants to merge 6 commits into from
Closed

RFC: Support for pack.toml #189

wants to merge 6 commits into from

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Nov 16, 2021

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner requested a review from a team as a code owner November 16, 2021 22:36
@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@loewenstein
Copy link
Contributor

I understand that e.g. builder might be difficult to impossible to support. On the other hand side, introducing this special handling for the platform maintained as part of CNB core feels a bit like opening the box of Pandora.

text/0000-pack-toml.md Outdated Show resolved Hide resolved
text/0000-pack-toml.md Outdated Show resolved Hide resolved
text/0000-pack-toml.md Show resolved Hide resolved
@hone
Copy link
Member

hone commented Jan 19, 2022

@jkutner do you mind if I move this to "blocked" on #191?

@hone hone added the status/blocked RFC is blocked by some other outstanding dependency. label Jan 19, 2022
@loewenstein
Copy link
Contributor

Is #191 the same thing that @jromero is currently drafting in https://hackmd.io/4LGBHOZAQ6GMOTeDf2U3_w?view#What-it-is? If yes, the existence of project.toml namespaces like io.buildpacks.pack.* leads me to the question if that would supersede the proposal for a pack specific configuration file...?

If I am right in this, then section about merging pack specific config seems to build a circle with this rfc that might be worth resolving ;)

Thoughts? @jromero @jkutner

@jkutner
Copy link
Member Author

jkutner commented Feb 3, 2022

@loewenstein I think this RFC started the conversation that led to @jromero's hackmd draft RFC. I'm supportive of that one, and will likely close this once we turn his into a real RFC

@loewenstein
Copy link
Contributor

@loewenstein I think this RFC started the conversation that led to @jromero's hackmd draft RFC. I'm supportive of that one, and will likely close this once we turn his into a real RFC

I was asking because the proposal of @jromero seems to try to accommodate this one ;)

@jromero
Copy link
Member

jromero commented Feb 7, 2022

@jkutner, I submitted the RFC for a Prepare Operation and excluded the section referring to the pack.toml specifically in order to keep the RFC as concise as possible. I'd like to propose that we continue to make changes to this RFC separately with the other RFC in mind.

text/0000-pack-toml.md Show resolved Hide resolved
text/0000-pack-toml.md Show resolved Hide resolved
text/0000-pack-toml.md Outdated Show resolved Hide resolved
@hone hone added scope/team RFC scoped to a sub-team as opposed to the entire project. and removed status/blocked RFC is blocked by some other outstanding dependency. labels Mar 23, 2022
@jromero
Copy link
Member

jromero commented Apr 21, 2022

@jkutner have you had a chance to review my latest comments?

@dfreilich your feedback would also be appreciated here since this seems rather impacting.

@jkutner jkutner closed this Aug 31, 2022
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner reopened this Oct 2, 2022
@jkutner jkutner requested a review from jromero October 2, 2022 13:21
@jkutner jkutner assigned hone and unassigned jromero Oct 2, 2022
@loewenstein
Copy link
Contributor

Has there been any decision with regards to additional file vs. some flexibility and additional tables in the existing project descriptor file?

@jkutner
Copy link
Member Author

jkutner commented Oct 4, 2022

@loewenstein no decision, but I'm reopening this PR for discussion. My hope is that this will allow us to introduce pack specific attributes in the pack.toml (like for multiple images) that would not necessarily be honored by all platforms. Is that something that might work for you?


The Pack CLI will honor both the `pack.toml` and `project.toml` file descriptors with the following rules:

* If one or more descriptors are provided on the command line they will all be merged and used.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
* If one or more descriptors are provided on the command line they will all be merged and used.
* If one or more descriptors are provided on the command line, all top-level keys will all be merged and the resulting descriptor will be used.

@loewenstein
Copy link
Contributor

@loewenstein no decision, but I'm reopening this PR for discussion. My hope is that this will allow us to introduce pack specific attributes in the pack.toml (like for multiple images) that would not necessarily be honored by all platforms. Is that something that might work for you?

I guess I'd still prefer to have "project" related configuration in a single file, i.e. the project.yaml file. With the sole exception of maybe entirely only pack concerning config, but I think pack has config for e.g. default builders already in the user home.

@jkutner
Copy link
Member Author

jkutner commented Oct 11, 2022

@loewenstein IIUC that matches the intent of this proposal. Any general project configuration can go in project.toml (including pack config if you want it there). Pack-only config can go in pack.toml. What's the gap from what you described?


```toml
[io.buildpacks.pack]
builder = "<string (optional)>"
Copy link
Member

Choose a reason for hiding this comment

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

io.buildpacks.builder already seems to be supported in project.toml here: https://github.com/buildpacks/spec/blob/main/extensions/project-descriptor.md#iobuildpacksbuilder-optional. Is this different somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that may have been added since I first wrote this. I'll update with a different example that shows something that's specific to pack (like tag or maybe env-file)

@hone
Copy link
Member

hone commented Dec 2, 2022

Moving this into FCP for next Thurs Dec 8.

@hone
Copy link
Member

hone commented Dec 2, 2022

@jkutner how does this play with $HOME/.pack/config.toml? For example here is what mine looks like:

default-builder-image = "heroku/buildpacks:20"
experimental = true

[[trusted-builders]]
  name = "ops0-artifactrepo1-0-prd.data.sfdc.net/docker-sam/buildpacks/sfdc_centos7_cnb:builder"

Would these be supported in pack.toml as a way to have app/project level overrides? I'm thinking something along the lines of how git-config does this.

@loewenstein
Copy link
Contributor

@loewenstein IIUC that matches the intent of this proposal. Any general project configuration can go in project.toml (including pack config if you want it there). Pack-only config can go in pack.toml. What's the gap from what you described?

I guess my main concern is with things that are maybe pack only today, but will be picked up by other platforms in the future. From my point of view, it would make sense for most of the configuration you would possible want to support to go through the project descriptor spec and potentially align support across platforms.

@jjbustamante
Copy link
Member

I really like the motivation: advertises that the repo containing the file can be built with pack

Now, I have the following doubt: Is there an idea to accomplish that? I mean, for example, if we think of developers that use vscode and they want some extension for Docker, then they have a lot of options. Are we considering creating or maintaining our own IDEs extensions for CNB tooling?

When multiple descriptors are _merged_ the following precedence is given:

* Configuration in a file provided on the command line will take precedence over files provided on the command line after it.
* Configuration in a `pack.toml` will take precedence over configuration in a `project.toml`
Copy link

Choose a reason for hiding this comment

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

I would like some outputs from pack to indicate the config being used (even better if it could trace it back to the origin and say why). That way users can know at a glance if their changes have been picked up or if they're not being picked up, get a hint that maybe another file is taking precedence.

I'm unsure whether that desire can be encoded in the RFC in a sensible way.

The Pack CLI will support a new descriptor file, `pack.toml`, that is a superset of the project descriptor.
* An app MAY have a `pack.toml`
* An app MAY have a `project.toml`
* An app MAY have both a `pack.toml` and a `project.toml`, in which case the `project.toml` will be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* An app MAY have both a `pack.toml` and a `project.toml`, in which case the `project.toml` will be ignored.
* An app MAY have both a `pack.toml` and a `project.toml` where they will be merged and used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hone we've gone back and forth on this. Merging could actually be very complicated, and if we support merging we should only do it at the top level shared key (i.e. we're not going to try and merge lists of things for example)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkutner this is precisely why I have a problem with a platform specific descriptor covering platform agnostic configuration.

I would want to have as much as possible in an agnostic way, at least from the perspective of all platforms other than pack. Why? Because for 99% of the non pack platforms I can imagine, pack will be the platform for local execution. See kpack and our integration into project Piper as two examples and Tekton most likely as a third.

Also, as I pointed out in #189 (comment) the “project.toml will be ignored” statement contradicts other parts of this proposal.

Copy link
Member

@sambhav sambhav left a comment

Choose a reason for hiding this comment

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

I still see multiple project descriptor related RFCs open. Can we consolidate them and figure out how each fits in the larger picture? I don't want there to be multiple ways of configuration as that just leads to complicated code and lots of corner cases. If we do decide to implement multiple related RFCs, I would also like to see how they interplay with each other and what precedence would look like amongst them.

text/0000-pack-toml.md Outdated Show resolved Hide resolved
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Copy link
Member Author

jkutner commented Dec 14, 2022

I'm going to close this RFC again. There is enough about merging pack.toml and project.toml that isn't clear, and there isn't enough interest in using pack.toml (even from me) to make me move it forward.

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.