New flatten buildpacks/builder implementation - Part 2 - Implementing RFC-0123

[jjbustamante]

Summary

This PR implements RFC-0123.

The main idea can be summarize with the following class diagram:

classDiagram

    ManagedCollection <|-- managedCollection
    managedCollection <|-- managedCollectionV1
    managedCollection <|-- managedCollectionV2

    class ManagedCollection {
        <<interface>>
    }

     ModuleInfo --* ModuleInfos
     ModuleInfos --*  FlattenModuleInfos
     managedCollectionV2 -- FlattenModuleInfos

    class ModuleInfo {
        FullName() string
    } 

    class ModuleInfos {
        <<interface>>
        BuildModule() []dist.ModuleInfo
    }

    class FlattenModuleInfos {
	    <<interface>>
        FlattenModules() []ModuleInfos
    }

    class managedCollection {
             +AllModules()
	    +ExplodedModules() 
	    +FlattenedModules()
	    +ShouldFlatten()
    }

    class managedCollectionV1 {
        +AddModules()
    }

    class managedCollectionV2 {
        +AddModules()

    }

Loading

Our first attempt to implement the flatten feature added a ManagedCollection structure, I extracted the methods to a new interface. managedCollection is a base implementation with all the common code for each implementation. The idea was to keep our initial implementation on managedCollectionV1 and implement the new behavior with managedCollectionV2.

According to the RFC, the user is responsible of defining which buildpacks they want to flatten. In order to represent this we added the FlattenModuleInfos interface. It is a two dimensional array with the group of modules we want to put together.

managedCollectionV2 uses FlattenModuleInfos to determine how to flatten the modules when they are added.

Output

Before

After

Documentation

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

Related

Resolves #1880

[natalieparellano]

@jjbustamante apologies for being so slow to review this one. In general it looks good to me - I made some suggestions to potentially help readability but otherwise it looks like it does what it's supposed to do. It might make sense to add an acceptance test if we don't have one already.

[natalieparellano]

Is there any way to combine these two interfaces? Is there a reason for them to be different?

[jjbustamante]

It is actually a way of having a two dimensional array.
FlattenModules() -> return an array of ModuleInfos where each element is an array of dist.ModuleInfo

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a51140e) 79.49% compared to head (c044cb7) 79.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1985      +/-   ##
==========================================
+ Coverage   79.49%   79.63%   +0.15%     
==========================================
  Files         175      176       +1     
  Lines       13135    13199      +64     
==========================================
+ Hits        10440    10510      +70     
+ Misses       2026     2020       -6     
  Partials      669      669              

Flag	Coverage Δ
os_linux	78.53% <98.43%> (+0.13%)	⬆️
os_macos	76.39% <98.43%> (?)
os_windows	79.02% <98.43%> (+0.15%)	⬆️
unit	79.63% <98.43%> (+0.15%)	⬆️

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

[jjbustamante]

• FlattenAll came from my first pull request (see Part 1)
• Because the RFC is changing the pack builder create interface, now the end-user must define which buildpacks must be flatten together, that's why I found more readable to use WithFlatten as name to the option method

[jjbustamante]

I think I should return an managedCollectionV1 instance here.

[jjbustamante]

The --flaten-exclude flag is being removed according to the RFC

[natalieparellano]

Nice work @jjbustamante. I left a few more nits - I think it could be helpful to add some more tests, but overall this is looking good.

[jjbustamante]

@natalieparellano .. if you can take one more look will be awesome

[natalieparellano]

Looks good to me :)

[edmorley]

I couldn't get this feature to work? I filed:
#2050

Also I filed a usability feature request:
#2051