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

pack build should support --extension #1551

Closed
1 task done
Tracked by #224
natalieparellano opened this issue Nov 10, 2022 · 9 comments · Fixed by #1651
Closed
1 task done
Tracked by #224

pack build should support --extension #1551

natalieparellano opened this issue Nov 10, 2022 · 9 comments · Fixed by #1651
Assignees
Labels
good first issue A good first issue to get started with. status/triage Issue or PR that requires contributor attention. type/enhancement Issue that requests a new feature or improvement.

Comments

@natalieparellano
Copy link
Member

natalieparellano commented Nov 10, 2022

Description

Today you can run pack build --buildpack to dynamically add buildpacks to the builder and modify the order. It should be possible to add extensions in the same way.

Proposed solution

  • --extensions should always come first
  • There is a separate order for extensions (order-extensions in order.toml) so we'll need to take that into account
  • This should be blocked on pack should support pack extension * commands #1470 (support for pack extension package) as --extension can accept a package input - we can make a separate issue for extension packages

Additional context

  • This feature should be documented somewhere
@natalieparellano natalieparellano added type/enhancement Issue that requests a new feature or improvement. status/triage Issue or PR that requires contributor attention. labels Nov 10, 2022
@natalieparellano natalieparellano added the good first issue A good first issue to get started with. label Feb 13, 2023
@edithwuly
Copy link
Contributor

Hi @natalieparellano, I would like to have a try at this issue.

@natalieparellano
Copy link
Member Author

That is awesome @edithwuly! I can assign the issue to you if you like. In case it's helpful, here are a few relevant places in the code:

  • Buildpacks []string
    - Buildpacks holds additional buildpacks from --buildpack; we'll likely want to make an Extensions
  • Buildpacks: buildpacks,
    - where we read in the build options
  • func (c *Client) processBuildpacks(ctx context.Context, builderImage imgutil.Image, builderBPs []dist.ModuleInfo, builderOrder dist.Order, stackID string, opts BuildOptions) (fetchedBPs []buildpack.BuildModule, order dist.Order, err error) {
    - where we download the additional buildpacks; we can likely re-use some of this function for extensions; however the project descriptor part is not relevant and instead of appending to bldr.Order(), we want to append to bldr.OrderExtensions()

Please don't hesitate to reach out if you have any questions!

@edithwuly
Copy link
Contributor

Thanks @natalieparellano! It will be great if you could assign this issue to me. And the places you share are exactly what I'm looking for. I will let you know as soon as there is progress.

@jjbustamante
Copy link
Member

Thanks a lot for helping us with this @edithwuly!

@edithwuly
Copy link
Contributor

edithwuly commented Feb 20, 2023

Hi @natalieparellano, I have looking into the code and have some idea about what to do.

  • Buildpacks []string

    • I'm going to create a field Extensions to hold additional extensions from --extension
  • func (c *Client) processBuildpacks(ctx context.Context, builderImage imgutil.Image, builderBPs []dist.ModuleInfo, builderOrder dist.Order, stackID string, opts BuildOptions) (fetchedBPs []buildpack.BuildModule, order dist.Order, err error) {

    • I'm going to create a function processExtensions() to fetch and order extensions. I believe I can re-use some code of processBuildpacks(). And I notice there are many conditions considered in processBuildpacks() like locatorType and opts.PreBuildpacks. Is there any condition I need to consider when processing extensions?
  • func (c *Client) createEphemeralBuilder(rawBuilderImage imgutil.Image, env map[string]string, order dist.Order, buildpacks []buildpack.BuildModule) (*builder.Builder, error) {

    • I'm going to pass the fetched extensions and order to createEphemeralBuilder(), and add them to the builder by invoking bldr.AddExtension() and bldr.SetOrderExtensions()

Is there anything wrong or missed?

@natalieparellano
Copy link
Member Author

@edithwuly that looks right to me! For processExtensions(), all LocatorType values are valid for extensions also, except for RegistryLocator. You don't need to worry about opts.PreBuildpacks, because there isn't an equivalent concept for extensions.

@edithwuly
Copy link
Contributor

@natalieparellano sounds great! Thanks for your help! I'm working on it.

@edithwuly
Copy link
Contributor

Hi @natalieparellano! I have completed a simple version and proposed a PR. Please help me review, thanks!

Besides, when developing, I have some questions:

  • About the order of extensions. I'm not sure whether the added extensions should be placed at first, and I just re-used prependBuildpackToOrder() since there is no pre or post concept for extensions.
  • I handle RegistryLocator in processExtensions() and re-used fetchBuildpack() to fetch extensions by just adding ModuleKind value in DownloadOptions structure. Is there any conditions I didn't consider?
  • Should I do some unit tests or end-to-end tests? I have tested this version for tutorial case on Buildpack official website. Maybe you can help me get more test cases!

@natalieparellano
Copy link
Member Author

@edithwuly thank you so much for the PR! I'll look this over tomorrow :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good first issue to get started with. status/triage Issue or PR that requires contributor attention. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants