pack build support --extension

[edithwuly]

Summary

pack build support --extension.

Output

Before

Error: unknown flag: --extension

After

...
Adding extension samples/tree version 0.0.1 to builder
...
Successfully built image sample-app

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #1551

[codecov]

Codecov Report

Merging #1651 (a5b92c2) into main (16d6187) will decrease coverage by 0.07%.
The diff coverage is 67.00%.

❗ Current head a5b92c2 differs from pull request most recent head fdcd17d. Consider uploading reports for the commit fdcd17d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1651      +/-   ##
==========================================
- Coverage   80.53%   80.45%   -0.07%     
==========================================
  Files         165      165              
  Lines       10850    10925      +75     
==========================================
+ Hits         8737     8789      +52     
- Misses       1587     1606      +19     
- Partials      526      530       +4     

Flag	Coverage Δ
os_linux	79.28% <67.00%> (-0.06%)	⬇️
os_macos	77.40% <67.00%> (-0.08%)	⬇️
os_windows	80.37% <67.00%> (-0.07%)	⬇️
unit	80.45% <67.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[natalieparellano]

@edithwuly awesome work! This is really great to see. The code looks sound, we just need to add some tests in order to verify. You might take a look at the tests for --buildpack here:

pack/pkg/client/build_test.go

Line 684 in deb588f

when("Buildpacks option", func() {

I don't think all of the tests in this block are valid for extensions, probably the most important ones are:

• pack/pkg/client/build_test.go

  Line 693 in deb588f

  it("builder order is overwritten", func() {

• pack/pkg/client/build_test.go

  Line 720 in deb588f

  when("id - no version is provided", func() {

• pack/pkg/client/build_test.go

  Line 1073 in deb588f

  it("all buildpacks are added to ephemeral builder", func() {

  
  This may also benefit from an acceptance test, like we have here - however even experienced contributors will agree that modifying the pack acceptance tests can be painful, so we could definitely help you with that or work to minimize the scope for this PR.

[natalieparellano]

A note, in case it helps others, buildpack.FromBuilderLocator is deprecated now that we have pre- and post- buildpacks. We may want --pre-extension and --post-extension (like exist for buildpacks) but that should be considered out-of-scope for this issue.

[edithwuly]

I have added case buildpack.FromBuilderLocator in processExtensions()` funtion. Please help me review, thanks!

[edithwuly]

Thanks @natalieparellano for your review! I'm glad to know my code is satisfying. I'm working on the most important unit tests and will patch this PR as soon as completed.

[edithwuly]

Hi @natalieparellano! I try to add two unit tests for --extension. Please help me review. Thanks！

But I have some trouble on the "all extensions are added to ephemeral builder" test. I think it is used to test whether child extension can be added to builder with meta extension as a whole layer. And it uses the extractPackaged() function, in which it seems that extractExtensions() function has not been ready.

pack/pkg/buildpack/downloader.go

Line 201 in 558ae9a

return nil, nil, nil // TODO: add extractExtensions when `pack extension package` is supported in https://github.com/buildpacks/pack/issues/1489

[natalieparellano]

@edithwuly awesome work! This is getting to be in great shape. I left a couple nits and one clarification. It seems almost done now :)

[natalieparellano]

@edithwuly any update on this one? This one just requires a couple of updates (and would make acceptance tests for #1698 a LOT easier 🙃 )

[edithwuly]

Yes, sorry about the delay. I have merged your suggestions, removed the rest three lines and merged the buildpacks:main. Thank you very much! @natalieparellano

[pacostas]

I tested this PR using the extension we've been working on, and it seems to be working properly. We're really excited about getting this PR landed :)