Fixing an parsing error with the buildpacks to be flattened

[jjbustamante]

Summary

Unfortunately with pack 0.33.0 I didn't realized spf13/cobra library was parsing a comma separated string input to created a []string already splited. As a consequence, when users tries to use the new --flatten flag implementation, when creating a builder, the group of buildpacks to be in the same layer was incorrectly created.

This PR changes the method use to parse the flag from spf13/cobra to be StringArrayVar which according to the documentation The value of each argument will not try to be separated by comma

Output

Before

See the original issue #2050 reported

After

pack builder create --verbose --config builder-22/builder.toml --flatten "heroku/gradle@4.1.0,heroku/java@4.1.0,heroku/jvm@4.1.0,heroku/maven@4.1.0,heroku/sbt@4.1.0,heroku/scala@4.1.0" builder-flattened

The logic to flattened is working as expected, using dive tool we can check the previous buildpack were put together in the same layer

Documentation

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

Related

Resolves #2050

[natalieparellano]

In func ParseFlattenBuildModules(buildpacksID []string, sep string) (FlattenModuleInfos, error) { can we not just get rid of sep entirely and expect a list? Then --flatten would have a nice parity with how we already handle lists like pack build --buildpack (note Repeat for each buildpack in order, or supply once by comma-separated list):

  -b, --buildpack strings            Buildpack to use. One of:
                                       a buildpack by id and version in the form of '<buildpack>@<version>',
                                       path to a buildpack directory (not supported on Windows),
                                       path/URL to a buildpack .tar or .tgz file, or
                                       a packaged buildpack image name in the form of '<hostname>/<repo>[:<tag>]'
                                     Repeat for each buildpack in order, or supply once by comma-separated list

It seems weird to sometimes expect commas and sometimes expect semicolons when providing list arguments to pack

[jjbustamante]

In func ParseFlattenBuildModules(buildpacksID []string, sep string) (FlattenModuleInfos, error) { can we not just get rid of sep entirely and expect a list? Then --flatten would have a nice parity with how we already handle lists like pack build --buildpack (note Repeat for each buildpack in order, or supply once by comma-separated list):

  -b, --buildpack strings            Buildpack to use. One of:
                                       a buildpack by id and version in the form of '<buildpack>@<version>',
                                       path to a buildpack directory (not supported on Windows),
                                       path/URL to a buildpack .tar or .tgz file, or
                                       a packaged buildpack image name in the form of '<hostname>/<repo>[:<tag>]'
                                     Repeat for each buildpack in order, or supply once by comma-separated list

It seems weird to sometimes expect commas and sometimes expect semicolons when providing list arguments to pack

Yeah, sorry for that, checking deeply the options in Cobra, I found that StringArrayVar was actually what I need!

[natalieparellano]

LGTM. We might want to add an acceptance test to ensure there aren't any regressions later

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (30dcc15) 79.63% compared to head (572f869) 79.63%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2053   +/-   ##
=======================================
  Coverage   79.63%   79.63%           
=======================================
  Files         176      176           
  Lines       13214    13214           
=======================================
  Hits        10522    10522           
  Misses       2022     2022           
  Partials      670      670           

Flag	Coverage Δ
os_linux	78.54% <100.00%> (-0.02%)	⬇️
os_macos	76.39% <100.00%> (ø)
os_windows	79.02% <100.00%> (ø)
unit	79.63% <100.00%> (ø)

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

[jjbustamante]

I didn't want to invest a lot of time refactoring the setup for a builder creation, this method is very similar to create complex builder, maybe it is time to continue Javier's work on refactoring the tests

[jjbustamante]

The test correctly failed when using previous pack version to create the builder. That's the reason the error was reported.