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 prepare operation RFC, continued #238

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

david-caro
Copy link

@david-caro david-caro commented Nov 18, 2022

This follows up on #202

Readable

Change Log

2022-11-18 (3bb05f8)

2022-02-22 (94e3180, d02754f)

2022-02-10 (8fc7a70)

  • Add a detailed summary

  • Give the io.buildpacks namespace an independent schema version

2022-02-07 (99fc145)

  • Initial RFC

jromero and others added 5 commits February 7, 2022 10:39
@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)

@natalieparellano
Copy link
Member

@jkutner @jabrown85 any thoughts on this?

@jkutner
Copy link
Member

jkutner commented Nov 30, 2022

I'd love to see this move forward. Thanks for opening it

@sambhav
Copy link
Member

sambhav commented Nov 30, 2022

In general one alternative I would to see reflected in this proposal and discussed is the the following interface -

preparer </path/to/lifecycle-config.toml>

where the lifecycle config is a viper config file that maps to the various lifecycle binary flags + env vars. (See https://github.com/spf13/viper#reading-config-files).

The preparer would take the above file and modify any of the inputs in the file and pass it back to the platform to be supplied to the lifecycle.

The above file would also need to include the platform API. This way the preparer becomes a part of the platform API and has a way of modifying all the inputs needed.

This can be used to implement all of the possible functionality we could ever want from a prepare stage and this interface would be forward-compatible as long as the preparer is compatible with a certain platform API.

Related #128

Maybe it's going too much into implementation details, but standardizing lifecycle input parsing on viper would allow us to expose a consistent format to map lifecycle binary CLI args <> config file <> lifecycle env vars and define an order of precedence for them.

Comment on lines +33 to +34
- **Goal 2: A recognizable file in repositories**
> As a user, I would like to be able to recognize, based on the file system, if a project is using Cloud Native Buildpacks.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand how this goal fits in

Copy link
Member

Choose a reason for hiding this comment

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

Is it because I can not depend on project.toml to work, regardless of platform?

Copy link
Author

Choose a reason for hiding this comment

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

I think so yes. The project descriptor file currently can be ignored by the platform in any case right?
This adds the restriction that now they MUST apply at least the io.buildpacks section, making the combo (project.toml + io.buildpacks) a way to identify if the project uses buildpacks, regardless of the platform.

So summarizing my own words, this RFC ensures that if there's a project.toml, and it has a `io.buildpacks' section, then you can rely on it using those buildpacks regardless of platform.

Does that make sense? (/me is still trying to understand both the original motivation and current flows, so please be extra verbose with the replies 🙏 )


Therefore the following changes have been made from the [latest schema][pd-02]:

- **Remove `builder` from `io.buildpacks` namespace**
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a plan for backwards compat?

Copy link
Author

Choose a reason for hiding this comment

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

Not that I see, I think it's relying on the schema-version (see below) to do that breaking change.

Do we need one? (I can add bot a backwards compatible/deprecation period and a removal procedure to the rfc)

# kpack config
##

[com.vmware.kpack]
Copy link
Member

Choose a reason for hiding this comment

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

the com.vmware is a bit problematic. Does kpack have a vendor neutral namespace?

Copy link
Author

Choose a reason for hiding this comment

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

I'm guessing that will depend on #235, but I'll look into it

- Set build env vars in `<platform>/env`
- Notify users of any other properties in `io.buildpacks`

# Drawbacks
Copy link
Member

Choose a reason for hiding this comment

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

Are there any drawbacks around efficiency? Like this change to excludes could impact performance, because it means that very large files that are excluded might still have to be sent to some remote platform that runs prepare and then removes the exclude files later


1. Parsing and applying the `io.buildpacks` namespace becomes responsibility of the platform.
- This is mitigated by providing [utilities](#Cloud-Native-Buildpacks-utilities) and the fact that the prepare phase is independent and swappable.
2. Executing the Prepare operation may require an additional container to be spun up in some platforms; this would effectively lengthen the overall build process.
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this will work with creator?

@david-caro
Copy link
Author

In general one alternative I would to see reflected in this proposal and discussed is the the following interface -

preparer </path/to/lifecycle-config.toml>

where the lifecycle config is a viper config file that maps to the various lifecycle binary flags + env vars. (See https://github.com/spf13/viper#reading-config-files).

The preparer would take the above file and modify any of the inputs in the file and pass it back to the platform to be supplied to the lifecycle.

The above file would also need to include the platform API. This way the preparer becomes a part of the platform API and has a way of modifying all the inputs needed.

This can be used to implement all of the possible functionality we could ever want from a prepare stage and this interface would be forward-compatible as long as the preparer is compatible with a certain platform API.

Related #128

Maybe it's going too much into implementation details, but standardizing lifecycle input parsing on viper would allow us to expose a consistent format to map lifecycle binary CLI args <> config file <> lifecycle env vars and define an order of precedence for them.

As I understand, this would introduce a new configuration file (the lifecycle-config.toml) and an new configuration file parsing strategy.

  • About using the new lifecycle-config.toml file, wouldn't that be duplicating what the project descriptor is for? (according to https://github.com/buildpacks/spec/blob/main/extensions/project-descriptor.md)

  • About using viper for config parsing and such, I think it would be great to consolidate on it yes :), but out of scope of this RFC? I would not mind working on it though, aside from this maybe. Do you think that just an issue would be enough or a new RFC is required? (I'm not very familiar with the processes of the group yet :} )

(btw. I can absolutely add the option you requested to the doc, just let me know, I'm trying to discuss/understand the details)


Therefore the following changes have been made from the [latest schema][pd-02]:

- **Remove `builder` from `io.buildpacks` namespace**
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pack support this key today? I think my vote would be for a platform to fail building (or warn, I guess) if the builder key isn't supported. But if we remove it entirely I suppose I won't be too upset. Platforms can use their own namespace to achieve the same thing.

@natalieparellano
Copy link
Member

@david-caro were you still interested in moving this one forward?

@david-caro
Copy link
Author

I am yes, though I'm quite busy lately, feel free to take over if you want to push it

@natalieparellano
Copy link
Member

@david-caro sounds good, I'll move this into draft for now. We can move it out of draft when someone has the bandwidth to pick it up :)

@natalieparellano natalieparellano marked this pull request as draft August 17, 2023 18:32
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.

7 participants