From 681c876736217d1092b1519c854aeb97ce1bdc76 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Thu, 25 May 2023 17:25:22 -0500 Subject: [PATCH 1/5] 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/5] 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 2e1b3a52343ba9ddde0976a14c4e77d21f882e3f Mon Sep 17 00:00:00 2001 From: Natalie Arellano Date: Tue, 30 May 2023 17:09:00 -0400 Subject: [PATCH 3/5] When deriving stack.toml from the new run image information 2 we should write the file with the correct schema Signed-off-by: Natalie Arellano --- internal/builder/builder.go | 2 +- internal/builder/builder_test.go | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 7b66ad891..a69f7d802 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -1083,7 +1083,7 @@ func (b *Builder) stackLayer(dest string) (string, error) { if b.metadata.Stack.RunImage.Image != "" { err = toml.NewEncoder(buf).Encode(b.metadata.Stack) } else if len(b.metadata.RunImages) > 0 { - err = toml.NewEncoder(buf).Encode(b.metadata.RunImages[0]) + err = toml.NewEncoder(buf).Encode(StackMetadata{RunImage: b.metadata.RunImages[0]}) } if err != nil { return "", errors.Wrapf(err, "failed to marshal stack.toml") diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index 26ad0c144..69cf7947d 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -1583,6 +1583,18 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) { ) }) + it("adds the stack.toml to the image", func() { + layerTar, err := baseImage.FindLayerWithPath("/cnb/stack.toml") + h.AssertNil(t, err) + h.AssertOnTarEntry(t, layerTar, "/cnb/stack.toml", + h.ContentEquals(`[run-image] + image = "some/run" + mirrors = ["some/mirror", "other/mirror"] +`), + h.HasModTime(archive.NormalizedDateTime), + ) + }) + it("adds the run image to the metadata", func() { label, err := baseImage.Label("io.buildpacks.builder.metadata") h.AssertNil(t, err) From 37747efaf195582f76037616337cdd338a0515e0 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 26 May 2023 08:46:46 -0500 Subject: [PATCH 4/5] 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.") }) }) }) From c84939bfd7a3fd9a94370442d384d16ea46bab23 Mon Sep 17 00:00:00 2001 From: Natalie Arellano Date: Thu, 15 Jun 2023 12:10:47 -0400 Subject: [PATCH 5/5] Fix small issue with deprecated BOM display Extensions don't output legacy BOMs so including them in the BOM display is a bit misleading. Signed-off-by: Natalie Arellano --- internal/inspectimage/bom_display.go | 8 -------- internal/inspectimage/writer/bom_json_test.go | 2 -- internal/inspectimage/writer/bom_yaml_test.go | 2 -- 3 files changed, 12 deletions(-) diff --git a/internal/inspectimage/bom_display.go b/internal/inspectimage/bom_display.go index e8a85c34e..28e273288 100644 --- a/internal/inspectimage/bom_display.go +++ b/internal/inspectimage/bom_display.go @@ -19,7 +19,6 @@ type BOMEntryDisplay struct { Version string `toml:"version,omitempty" json:"version,omitempty" yaml:"version,omitempty"` Metadata map[string]interface{} `toml:"metadata" json:"metadata" yaml:"metadata"` Buildpack dist.ModuleRef `json:"buildpacks" yaml:"buildpacks" toml:"buildpacks"` - Extension dist.ModuleRef `json:"extensions" yaml:"extensions" toml:"extensions"` } func NewBOMDisplay(info *client.ImageInfo) []BOMEntryDisplay { @@ -68,13 +67,6 @@ func displayBOMWithExtension(bom []buildpack.BOMEntry) []BOMEntryDisplay { }, Optional: entry.Buildpack.Optional, }, - Extension: dist.ModuleRef{ - ModuleInfo: dist.ModuleInfo{ - ID: entry.Buildpack.ID, - Version: entry.Buildpack.Version, - }, - Optional: entry.Buildpack.Optional, - }, }) } diff --git a/internal/inspectimage/writer/bom_json_test.go b/internal/inspectimage/writer/bom_json_test.go index 9ff336333..5e0b8f8fb 100644 --- a/internal/inspectimage/writer/bom_json_test.go +++ b/internal/inspectimage/writer/bom_json_test.go @@ -45,7 +45,6 @@ func testJSONBOM(t *testing.T, when spec.G, it spec.S) { } } }, - "extensions": {}, "buildpacks": { "id": "test.bp.one.remote", "version": "1.0.0" @@ -68,7 +67,6 @@ func testJSONBOM(t *testing.T, when spec.G, it spec.S) { } } }, - "extensions": {}, "buildpacks": { "id": "test.bp.one.remote", "version": "1.0.0" diff --git a/internal/inspectimage/writer/bom_yaml_test.go b/internal/inspectimage/writer/bom_yaml_test.go index 4f7ae982c..855d06516 100644 --- a/internal/inspectimage/writer/bom_yaml_test.go +++ b/internal/inspectimage/writer/bom_yaml_test.go @@ -41,7 +41,6 @@ local: int: 456 nested: string: '' - extensions: {} buildpacks: id: test.bp.one.remote version: 1.0.0 @@ -57,7 +56,6 @@ remote: int: 123 nested: string: anotherString - extensions: {} buildpacks: id: test.bp.one.remote version: 1.0.0