From 681c876736217d1092b1519c854aeb97ce1bdc76 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Thu, 25 May 2023 17:25:22 -0500 Subject: [PATCH 1/3] Moving buildpack package --flatten command to be experimental. We still need how it affects the distribution spec Signed-off-by: Juan Bustamante --- internal/commands/buildpack_package.go | 27 ++++++++++++++++----- internal/commands/buildpack_package_test.go | 20 ++++++++++++--- internal/commands/package_buildpack.go | 2 +- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/internal/commands/buildpack_package.go b/internal/commands/buildpack_package.go index 50fe2a132..d772ef578 100644 --- a/internal/commands/buildpack_package.go +++ b/internal/commands/buildpack_package.go @@ -54,7 +54,7 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa "and they can be included in the configs used in `pack builder create` and `pack buildpack package`. For more " + "on how to package a buildpack, see: https://buildpacks.io/docs/buildpack-author-guide/package-a-buildpack/.", RunE: logError(logger, func(cmd *cobra.Command, args []string) error { - if err := validateBuildpackPackageFlags(&flags); err != nil { + if err := validateBuildpackPackageFlags(cfg, &flags); err != nil { return err } @@ -96,6 +96,10 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa logger.Warnf("%s is not a valid extension for a packaged buildpack. Packaged buildpacks must have a %s extension", style.Symbol(ext), style.Symbol(client.CNBExtension)) } } + if flags.Flatten { + logger.Warn("Creating a flattened Buildpack package could break the distribution specification. Please use it with caution.") + } + if err := packager.PackageBuildpack(cmd.Context(), client.PackageBuildpackOptions{ RelativeBaseDir: relativeBaseDir, Name: name, @@ -134,11 +138,16 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa cmd.Flags().BoolVar(&flags.Flatten, "flatten", false, "Flatten the buildpack into a single layer") cmd.Flags().StringSliceVarP(&flags.FlattenExclude, "flatten-exclude", "e", nil, "Buildpacks to exclude from flattening, in the form of '@'") cmd.Flags().IntVar(&flags.Depth, "depth", -1, "Max depth to flatten.\nOmission of this flag or values < 0 will flatten the entire tree.") + if !cfg.Experimental { + cmd.Flags().MarkHidden("flatten") + cmd.Flags().MarkHidden("depth") + cmd.Flags().MarkHidden("flatten-exclude") + } AddHelpFlag(cmd, "package") return cmd } -func validateBuildpackPackageFlags(p *BuildpackPackageFlags) error { +func validateBuildpackPackageFlags(cfg config.Config, p *BuildpackPackageFlags) error { if p.Publish && p.Policy == image.PullNever.String() { return errors.Errorf("--publish and --pull-policy never cannot be used together. The --publish flag requires the use of remote images.") } @@ -146,10 +155,16 @@ func validateBuildpackPackageFlags(p *BuildpackPackageFlags) error { return errors.Errorf("--config and --path cannot be used together. Please specify the relative path to the Buildpack directory in the package config file.") } - if p.Flatten && len(p.FlattenExclude) > 0 { - for _, exclude := range p.FlattenExclude { - if strings.Count(exclude, "@") != 1 { - return errors.Errorf("invalid format %s; please use '@' to exclude buildpack from flattening", exclude) + if p.Flatten { + if !cfg.Experimental { + return client.NewExperimentError("Flattening a buildpack package currently experimental.") + } + + if len(p.FlattenExclude) > 0 { + for _, exclude := range p.FlattenExclude { + if strings.Count(exclude, "@") != 1 { + return errors.Errorf("invalid format %s; please use '@' to exclude buildpack from flattening", exclude) + } } } } diff --git a/internal/commands/buildpack_package_test.go b/internal/commands/buildpack_package_test.go index dded63bf9..6b0c0babe 100644 --- a/internal/commands/buildpack_package_test.go +++ b/internal/commands/buildpack_package_test.go @@ -121,13 +121,25 @@ func testPackageCommand(t *testing.T, when spec.G, it spec.S) { }) }) when("flatten is set to true", func() { - when("flatten exclude doesn't have format @", func() { + when("experimental is true", func() { + when("flatten exclude doesn't have format @", func() { + it("errors with a descriptive message", func() { + cmd := packageCommand(withClientConfig(config.Config{Experimental: true}), withBuildpackPackager(fakeBuildpackPackager)) + cmd.SetArgs([]string{"test", "-f", "file", "--flatten", "--flatten-exclude", "some-buildpack"}) + + err := cmd.Execute() + h.AssertError(t, err, fmt.Sprintf("invalid format %s; please use '@' to exclude buildpack from flattening", "some-buildpack")) + }) + }) + }) + + when("experimental is false", func() { it("errors with a descriptive message", func() { - cmd := packageCommand(withBuildpackPackager(fakeBuildpackPackager)) - cmd.SetArgs([]string{"test", "-f", "file", "--flatten", "--flatten-exclude", "some-buildpack"}) + cmd := packageCommand(withClientConfig(config.Config{Experimental: false}), withBuildpackPackager(fakeBuildpackPackager)) + cmd.SetArgs([]string{"test", "-f", "file", "--flatten"}) err := cmd.Execute() - h.AssertError(t, err, fmt.Sprintf("invalid format %s; please use '@' to exclude buildpack from flattening", "some-buildpack")) + h.AssertError(t, err, "Flattening a buildpack package currently experimental.") }) }) }) diff --git a/internal/commands/package_buildpack.go b/internal/commands/package_buildpack.go index c0dfac260..e25183336 100644 --- a/internal/commands/package_buildpack.go +++ b/internal/commands/package_buildpack.go @@ -33,7 +33,7 @@ func PackageBuildpack(logger logging.Logger, cfg config.Config, packager Buildpa RunE: logError(logger, func(cmd *cobra.Command, args []string) error { deprecationWarning(logger, "package-buildpack", "buildpack package") - if err := validateBuildpackPackageFlags(&flags); err != nil { + if err := validateBuildpackPackageFlags(cfg, &flags); err != nil { return err } From d4545c7a585570598ea70d92a0d9095db020188e Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 26 May 2023 08:45:47 -0500 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Natalie Arellano Signed-off-by: Juan Bustamante --- internal/commands/buildpack_package.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/commands/buildpack_package.go b/internal/commands/buildpack_package.go index d772ef578..f33107a63 100644 --- a/internal/commands/buildpack_package.go +++ b/internal/commands/buildpack_package.go @@ -97,7 +97,7 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa } } if flags.Flatten { - logger.Warn("Creating a flattened Buildpack package could break the distribution specification. Please use it with caution.") + logger.Warn("Flattening a buildpack package could break the distribution specification. Please use it with caution.") } if err := packager.PackageBuildpack(cmd.Context(), client.PackageBuildpackOptions{ @@ -157,7 +157,7 @@ func validateBuildpackPackageFlags(cfg config.Config, p *BuildpackPackageFlags) if p.Flatten { if !cfg.Experimental { - return client.NewExperimentError("Flattening a buildpack package currently experimental.") + return client.NewExperimentError("Flattening a buildpack package is currently experimental.") } if len(p.FlattenExclude) > 0 { From 37747efaf195582f76037616337cdd338a0515e0 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 26 May 2023 08:46:46 -0500 Subject: [PATCH 3/3] Fixing unit test after updating the error message Adding test case for warning message Signed-off-by: Juan Bustamante --- internal/commands/buildpack_package_test.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/internal/commands/buildpack_package_test.go b/internal/commands/buildpack_package_test.go index 6b0c0babe..838c7a6dd 100644 --- a/internal/commands/buildpack_package_test.go +++ b/internal/commands/buildpack_package_test.go @@ -131,6 +131,23 @@ func testPackageCommand(t *testing.T, when spec.G, it spec.S) { h.AssertError(t, err, fmt.Sprintf("invalid format %s; please use '@' to exclude buildpack from flattening", "some-buildpack")) }) }) + + when("no exclusions", func() { + it("creates package with correct image name and warns flatten is being used", func() { + cmd := packageCommand( + withClientConfig(config.Config{Experimental: true}), + withBuildpackPackager(fakeBuildpackPackager), + withLogger(logger), + ) + cmd.SetArgs([]string{"my-flatten-image", "-f", "file", "--flatten"}) + err := cmd.Execute() + h.AssertNil(t, err) + + receivedOptions := fakeBuildpackPackager.CreateCalledWithOptions + h.AssertEq(t, receivedOptions.Name, "my-flatten-image.cnb") + h.AssertContains(t, outBuf.String(), "Flattening a buildpack package could break the distribution specification. Please use it with caution.") + }) + }) }) when("experimental is false", func() { @@ -139,7 +156,7 @@ func testPackageCommand(t *testing.T, when spec.G, it spec.S) { cmd.SetArgs([]string{"test", "-f", "file", "--flatten"}) err := cmd.Execute() - h.AssertError(t, err, "Flattening a buildpack package currently experimental.") + h.AssertError(t, err, "Flattening a buildpack package is currently experimental.") }) }) })