-
Notifications
You must be signed in to change notification settings - Fork 286
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
Fixing an parsing error with the buildpacks to be flattened #2053
Conversation
9421516
to
574fbae
Compare
In
It seems weird to sometimes expect commas and sometimes expect semicolons when providing list arguments to |
5808e7f
to
94b4d68
Compare
Yeah, sorry for that, checking deeply the options in Cobra, I found that |
LGTM. We might want to add an acceptance test to ensure there aren't any regressions later |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -3315,6 +3364,157 @@ func createStackImage(dockerCli client.CommonAPIClient, repoName string, dir str | |||
})) | |||
} | |||
|
|||
func createFlattenBuilder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Using StringArrayVar Signed-off-by: Juan Bustamante <[email protected]>
e2f47d4
to
f24d3bc
Compare
Signed-off-by: Juan Bustamante <[email protected]>
|
||
when("builder create", func() { | ||
when("--flatten=<buildpacks>", func() { | ||
it("should flatten together all specified buildpacks", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test correctly failed when using previous pack version to create the builder. That's the reason the error was reported.
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 documentationThe 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/[email protected],heroku/[email protected],heroku/[email protected],heroku/[email protected],heroku/[email protected],heroku/[email protected]" 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
Related
Resolves #2050