From 0ca0429d8e062b5814e52ba7257b71e78becef5c Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Wed, 22 May 2024 11:57:15 -0500 Subject: [PATCH] adding tests for builder create and improving logging messages Signed-off-by: Juan Bustamante --- acceptance/acceptance_test.go | 179 ++++++++++++++++++ acceptance/assertions/output.go | 6 + .../builder_multi_platform-no-targets.toml | 19 ++ .../pack_fixtures/builder_multi_platform.toml | 28 +++ internal/commands/builder_create.go | 2 +- internal/commands/builder_create_test.go | 127 +++++++------ internal/commands/buildpack_package.go | 8 +- pkg/client/client.go | 4 +- pkg/client/create_builder.go | 6 +- pkg/client/package_buildpack_test.go | 68 ++++--- pkg/image/fetcher.go | 5 +- 11 files changed, 350 insertions(+), 102 deletions(-) create mode 100644 acceptance/testdata/pack_fixtures/builder_multi_platform-no-targets.toml create mode 100644 acceptance/testdata/pack_fixtures/builder_multi_platform.toml diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 93adb4841..d166b12cc 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -3296,6 +3296,185 @@ include = [ "*.jar", "media/mountain.jpg", "/media/person.png", ] }) }) }) + + when("multi-platform", func() { + var ( + tmpDir string + multiArchBuildpackPackage string + builderTomlPath string + remoteRunImage string + remoteBuildImage string + err error + ) + + it.Before(func() { + h.SkipIf(t, !pack.SupportsFeature(invoke.MultiPlatformBuildersAndBuildPackages), "multi-platform builders and buildpack packages are available since 0.34.0") + + tmpDir, err = os.MkdirTemp("", "multi-platform-builder-create-tests") + assert.Nil(err) + + // used to avoid authentication issues with the local registry + os.Setenv("DOCKER_CONFIG", registryConfig.DockerConfigDir) + + // create a multi-platform buildpack and push it to a registry + multiArchBuildpackPackage = registryConfig.RepoName("simple-multi-platform-buildpack" + h.RandString(8)) + sourceDir := filepath.Join("testdata", "mock_buildpacks") + path := filepath.Join(tmpDir, "simple-layers-buildpack") + err = buildpacks.BpFolderSimpleLayers.Prepare(sourceDir, tmpDir) + h.AssertNil(t, err) + + output := pack.RunSuccessfully( + "buildpack", "package", multiArchBuildpackPackage, + "--path", path, + "--publish", + "--target", "linux/amd64", + "--target", "windows/amd64", + ) + assertions.NewOutputAssertionManager(t, output).ReportsPackagePublished(multiArchBuildpackPackage) + assertions.NewOutputAssertionManager(t, output).ReportsSuccessfulIndexPushed(multiArchBuildpackPackage) + h.AssertRemoteImageIndex(t, multiArchBuildpackPackage, types.OCIImageIndex, 2) + + // runImage and buildImage are saved in the daemon, for this test we want them to be available in a registry + remoteRunImage = registryConfig.RepoName(runImage + h.RandString(8)) + remoteBuildImage = registryConfig.RepoName(buildImage + h.RandString(8)) + + imageManager.TagImage(runImage, remoteRunImage) + imageManager.TagImage(buildImage, remoteBuildImage) + + h.AssertNil(t, h.PushImage(dockerCli, remoteRunImage, registryConfig)) + h.AssertNil(t, h.PushImage(dockerCli, remoteBuildImage, registryConfig)) + }) + + it.After(func() { + imageManager.CleanupImages(remoteBuildImage) + imageManager.CleanupImages(remoteRunImage) + os.RemoveAll(tmpDir) + }) + + generateMultiPlatformBuilderToml := func(template, buildpackURI, buildImage, runImage string) string { + t.Helper() + buildpackToml, err := os.CreateTemp(tmpDir, "buildpack-*.toml") + assert.Nil(err) + + pack.FixtureManager().TemplateFixtureToFile( + template, + buildpackToml, + map[string]interface{}{ + "BuildpackURI": buildpackURI, + "BuildImage": buildImage, + "RunImage": runImage, + }, + ) + assert.Nil(buildpackToml.Close()) + return buildpackToml.Name() + } + + when("builder.toml has no targets but the user provides --target", func() { + when("--publish", func() { + it.Before(func() { + builderName = registryConfig.RepoName("remote-multi-platform-builder" + h.RandString(8)) + + // We need to configure our builder.toml with image references that points to our ephemeral registry + builderTomlPath = generateMultiPlatformBuilderToml("builder_multi_platform-no-targets.toml", multiArchBuildpackPackage, remoteBuildImage, remoteRunImage) + }) + + it("publishes builder images for each requested target to the registry and creates an image index", func() { + output := pack.RunSuccessfully( + "builder", "create", builderName, + "--config", builderTomlPath, + "--publish", + "--target", "linux/amd64", + "--target", "windows/amd64", + ) + + defer imageManager.CleanupImages(builderName) + assertions.NewOutputAssertionManager(t, output).ReportsBuilderCreated(builderName) + + assertImage.CanBePulledFromRegistry(builderName) + + assertions.NewOutputAssertionManager(t, output).ReportsSuccessfulIndexPushed(builderName) + h.AssertRemoteImageIndex(t, builderName, types.OCIImageIndex, 2) + }) + }) + + when("--daemon", func() { + it.Before(func() { + builderName = registryConfig.RepoName("local-multi-platform-builder" + h.RandString(8)) + + // We need to configure our builder.toml with image references that points to our ephemeral registry + builderTomlPath = generateMultiPlatformBuilderToml("builder_multi_platform-no-targets.toml", multiArchBuildpackPackage, buildImage, runImage) + }) + + it("publishes builder image to the daemon for the given target", func() { + platform := "linux/amd64" + if runtime.GOOS == "windows" { + platform = "windows/amd64" + } + + output := pack.RunSuccessfully( + "builder", "create", builderName, + "--config", builderTomlPath, + "--target", platform, + ) + + defer imageManager.CleanupImages(builderName) + assertions.NewOutputAssertionManager(t, output).ReportsBuilderCreated(builderName) + }) + }) + }) + + when("builder.toml has targets", func() { + when("--publish", func() { + it.Before(func() { + builderName = registryConfig.RepoName("remote-multi-platform-builder" + h.RandString(8)) + + // We need to configure our builder.toml with image references that points to our ephemeral registry + builderTomlPath = generateMultiPlatformBuilderToml("builder_multi_platform.toml", multiArchBuildpackPackage, remoteBuildImage, remoteRunImage) + }) + + it("publishes builder images for each configured target to the registry and creates an image index", func() { + output := pack.RunSuccessfully( + "builder", "create", builderName, + "--config", builderTomlPath, + "--publish", + ) + + defer imageManager.CleanupImages(builderName) + assertions.NewOutputAssertionManager(t, output).ReportsBuilderCreated(builderName) + + assertImage.CanBePulledFromRegistry(builderName) + + assertions.NewOutputAssertionManager(t, output).ReportsSuccessfulIndexPushed(builderName) + h.AssertRemoteImageIndex(t, builderName, types.OCIImageIndex, 2) + }) + }) + + when("--daemon", func() { + it.Before(func() { + builderName = registryConfig.RepoName("local-multi-platform-builder" + h.RandString(8)) + + // We need to configure our builder.toml with image references that points to our ephemeral registry + builderTomlPath = generateMultiPlatformBuilderToml("builder_multi_platform.toml", multiArchBuildpackPackage, buildImage, runImage) + }) + + it("publishes builder image to the daemon for the given target", func() { + platform := "linux/amd64" + if runtime.GOOS == "windows" { + platform = "windows/amd64" + } + + output := pack.RunSuccessfully( + "builder", "create", builderName, + "--config", builderTomlPath, + "--target", platform, + ) + + defer imageManager.CleanupImages(builderName) + assertions.NewOutputAssertionManager(t, output).ReportsBuilderCreated(builderName) + }) + }) + }) + }) }) when("builder create", func() { diff --git a/acceptance/assertions/output.go b/acceptance/assertions/output.go index 65f206a86..d64be0e17 100644 --- a/acceptance/assertions/output.go +++ b/acceptance/assertions/output.go @@ -173,6 +173,12 @@ func (o OutputAssertionManager) IncludesUsagePrompt() { o.assert.Contains(o.output, "Run 'pack --help' for usage.") } +func (o OutputAssertionManager) ReportsBuilderCreated(name string) { + o.testObject.Helper() + + o.assert.ContainsF(o.output, "Successfully created builder image '%s'", name) +} + func (o OutputAssertionManager) ReportsSettingDefaultBuilder(name string) { o.testObject.Helper() diff --git a/acceptance/testdata/pack_fixtures/builder_multi_platform-no-targets.toml b/acceptance/testdata/pack_fixtures/builder_multi_platform-no-targets.toml new file mode 100644 index 000000000..8e6998332 --- /dev/null +++ b/acceptance/testdata/pack_fixtures/builder_multi_platform-no-targets.toml @@ -0,0 +1,19 @@ +[[buildpacks]] +id = "simple/layers" +version = "simple-layers-version" +uri = "{{ .BuildpackURI }}" + +[[order]] +[[order.group]] +id = "simple/layers" +version = "simple-layers-version" + +[build] +image = "{{ .BuildImage }}" + +[run] +[[run.images]] +image = "{{ .RunImage }}" + + + diff --git a/acceptance/testdata/pack_fixtures/builder_multi_platform.toml b/acceptance/testdata/pack_fixtures/builder_multi_platform.toml new file mode 100644 index 000000000..d67d82051 --- /dev/null +++ b/acceptance/testdata/pack_fixtures/builder_multi_platform.toml @@ -0,0 +1,28 @@ +[[buildpacks]] +id = "simple/layers" +version = "simple-layers-version" +uri = "{{ .BuildpackURI }}" + +[[order]] +[[order.group]] +id = "simple/layers" +version = "simple-layers-version" + +# Targets the buildpack will work with +[[targets]] +os = "linux" +arch = "amd64" + +[[targets]] +os = "windows" +arch = "arm64" + +[build] +image = "{{ .BuildImage }}" + +[run] +[[run.images]] +image = "{{ .RunImage }}" + + + diff --git a/internal/commands/builder_create.go b/internal/commands/builder_create.go index 3730dbc2e..0893c974d 100644 --- a/internal/commands/builder_create.go +++ b/internal/commands/builder_create.go @@ -94,7 +94,7 @@ Creating a custom builder allows you to control what buildpacks are used and wha } if len(multiArchCfg.Targets()) == 0 { - logger.Warnf("A new '--target' flag is available to set the platform") + logger.Infof("Pro tip: use --targets flag OR [[targets]] in builder.toml to specify the desired platform") } imageName := args[0] diff --git a/internal/commands/builder_create_test.go b/internal/commands/builder_create_test.go index 54c5e524d..db8baeda0 100644 --- a/internal/commands/builder_create_test.go +++ b/internal/commands/builder_create_test.go @@ -463,79 +463,94 @@ func testCreateCommand(t *testing.T, when spec.G, it spec.S) { }) when("multi-platform builder is expected to be created", func() { - when("--target", func() { - when("builder config has no targets defined", func() { - it.Before(func() { - h.AssertNil(t, os.WriteFile(builderConfigPath, []byte(validConfig), 0666)) - }) - when("daemon", func() { - it("errors when exporting to daemon", func() { - command.SetArgs([]string{ - "some/builder", - "--config", builderConfigPath, - "--target", "linux/amd64", - "--target", "windows/amd64", - }) - err := command.Execute() - h.AssertNotNil(t, err) - h.AssertError(t, err, "when exporting to daemon only one target is allowed") + when("builder config has no targets defined", func() { + it.Before(func() { + h.AssertNil(t, os.WriteFile(builderConfigPath, []byte(validConfig), 0666)) + }) + when("daemon", func() { + it("errors when exporting to daemon", func() { + command.SetArgs([]string{ + "some/builder", + "--config", builderConfigPath, + "--target", "linux/amd64", + "--target", "windows/amd64", }) + err := command.Execute() + h.AssertNotNil(t, err) + h.AssertError(t, err, "when exporting to daemon only one target is allowed") }) + }) - when("--publish", func() { - it.Before(func() { - mockClient.EXPECT().CreateBuilder(gomock.Any(), EqCreateBuilderOptionsTargets([]dist.Target{ - {OS: "linux", Arch: "amd64"}, - {OS: "windows", Arch: "amd64"}, - })).Return(nil) - }) + when("--publish", func() { + it.Before(func() { + mockClient.EXPECT().CreateBuilder(gomock.Any(), EqCreateBuilderOptionsTargets([]dist.Target{ + {OS: "linux", Arch: "amd64"}, + {OS: "windows", Arch: "amd64"}, + })).Return(nil) + }) - it("creates a builder with the given targets", func() { - command.SetArgs([]string{ - "some/builder", - "--config", builderConfigPath, - "--target", "linux/amd64", - "--target", "windows/amd64", - "--publish", - }) - h.AssertNil(t, command.Execute()) + it("creates a builder with the given targets", func() { + command.SetArgs([]string{ + "some/builder", + "--config", builderConfigPath, + "--target", "linux/amd64", + "--target", "windows/amd64", + "--publish", }) + h.AssertNil(t, command.Execute()) }) }) + }) + + when("builder config has targets defined", func() { + it.Before(func() { + h.AssertNil(t, os.WriteFile(builderConfigPath, []byte(validConfigWithTargets), 0666)) + }) - when("builder config has targets defined", func() { + when("--publish", func() { it.Before(func() { - h.AssertNil(t, os.WriteFile(builderConfigPath, []byte(validConfigWithTargets), 0666)) + mockClient.EXPECT().CreateBuilder(gomock.Any(), EqCreateBuilderOptionsTargets([]dist.Target{ + {OS: "linux", Arch: "amd64"}, + {OS: "linux", Arch: "arm64"}, + })).Return(nil) }) - when("--publish", func() { - it.Before(func() { - mockClient.EXPECT().CreateBuilder(gomock.Any(), EqCreateBuilderOptionsTargets([]dist.Target{ - {OS: "linux", Arch: "amd64"}, - {OS: "linux", Arch: "arm64"}, - })).Return(nil) + it("creates a builder with the given targets", func() { + command.SetArgs([]string{ + "some/builder", + "--config", builderConfigPath, + "--publish", }) + h.AssertNil(t, command.Execute()) + }) + }) - it("creates a builder with the given targets", func() { - command.SetArgs([]string{ - "some/builder", - "--config", builderConfigPath, - "--publish", - }) - h.AssertNil(t, command.Execute()) + when("invalid target flag is used", func() { + it("errors with a message when invalid target flag is used", func() { + command.SetArgs([]string{ + "some/builder", + "--config", builderConfigPath, + "--target", "something/wrong", + "--publish", }) + h.AssertNotNil(t, command.Execute()) + }) + }) + + when("--targets", func() { + it.Before(func() { + mockClient.EXPECT().CreateBuilder(gomock.Any(), EqCreateBuilderOptionsTargets([]dist.Target{ + {OS: "linux", Arch: "amd64"}, + })).Return(nil) }) - when("invalid target flag is used", func() { - it("errors with a message when invalid target flag is used", func() { - command.SetArgs([]string{ - "some/builder", - "--config", builderConfigPath, - "--target", "something/wrong", - "--publish", - }) - h.AssertNotNil(t, command.Execute()) + it("creates a builder with the given targets", func() { + command.SetArgs([]string{ + "some/builder", + "--target", "linux/amd64", + "--config", builderConfigPath, }) + h.AssertNil(t, command.Execute()) }) }) }) diff --git a/internal/commands/buildpack_package.go b/internal/commands/buildpack_package.go index 88f87e6f4..bfa2b09dc 100644 --- a/internal/commands/buildpack_package.go +++ b/internal/commands/buildpack_package.go @@ -108,6 +108,7 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa if err != nil { return err } + daemon := !flags.Publish && flags.Format == "" multiArchCfg, err := processMultiArchitectureConfig(logger, flags.Targets, targets, daemon) if err != nil { @@ -115,8 +116,13 @@ func BuildpackPackage(logger logging.Logger, cfg config.Config, packager Buildpa } if len(multiArchCfg.Targets()) == 0 { - logger.Warnf("A new '--target' flag is available to set the platform, using '%s' as default", bpPackageCfg.Platform.OS) + if isCompositeBP { + logger.Infof("Pro tip: use --targets flag OR [[targets]] in package.toml to specify the desired platform (os/arch/variant); using os %s", style.Symbol(bpPackageCfg.Platform.OS)) + } else { + logger.Infof("Pro tip: use --targets flag OR [[targets]] in buildpack.toml to specify the desired platform (os/arch/variant); using os %s", style.Symbol(bpPackageCfg.Platform.OS)) + } } else if !isCompositeBP { + // FIXME: Check if we can copy the config files during layers creation. filesToClean, err := multiArchCfg.CopyConfigFiles(bpPath) if err != nil { return err diff --git a/pkg/client/client.go b/pkg/client/client.go index 27370cb35..2f2bc9806 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -316,10 +316,10 @@ type imageFactory struct { } func (f *imageFactory) NewImage(repoName string, daemon bool, target dist.Target) (imgutil.Image, error) { - platform := imgutil.Platform{OS: target.OS, Architecture: target.Arch} + platform := imgutil.Platform{OS: target.OS, Architecture: target.Arch, Variant: target.ArchVariant} - platform.Variant = target.ArchVariant if len(target.Distributions) > 0 { + // We need to set platform distribution information so that it will be reflected in the image config. // We assume the given target's distributions were already expanded, we should be dealing with just 1 distribution name and version. platform.OSVersion = target.Distributions[0].Version } diff --git a/pkg/client/create_builder.go b/pkg/client/create_builder.go index ac58b257b..ca99c9921 100644 --- a/pkg/client/create_builder.go +++ b/pkg/client/create_builder.go @@ -430,10 +430,8 @@ func (c *Client) uriFromLifecycleVersion(version semver.Version, os string, arch if builder.SupportedLinuxArchitecture(architecture) { arch = architecture } else { - // TODO: Do we want to fail in this case instead of a warning? - // I added the warning to help the buildpack authors understand if they builder ended up with a wrong - // lifecycle binary. - c.logger.Warnf("trying to download a lifecycle binary for an unsupported architecture: %s, using default %s", style.Symbol(architecture), style.Symbol(arch)) + // FIXME: this should probably be an error case in the future, see https://github.com/buildpacks/pack/issues/2163 + c.logger.Warnf("failed to find a lifecycle binary for requested architecture %s, defaulting to %s", style.Symbol(architecture), style.Symbol(arch)) } return fmt.Sprintf("https://github.com/buildpacks/lifecycle/releases/download/v%s/lifecycle-v%s+linux.%s.tgz", version.String(), version.String(), arch) diff --git a/pkg/client/package_buildpack_test.go b/pkg/client/package_buildpack_test.go index 3cdaffd6a..7d92ef589 100644 --- a/pkg/client/package_buildpack_test.go +++ b/pkg/client/package_buildpack_test.go @@ -553,47 +553,45 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { }) when("folder structure doesn't follow multi-platform convention", func() { - when("no errors pushing the index", func() { - it.Before(func() { - destBpPath := filepath.Join("testdata", "buildpack-multi-platform", "buildpack-old-format") - bpPathURI, err = paths.FilePathToURI(destBpPath, "") + it.Before(func() { + destBpPath := filepath.Join("testdata", "buildpack-multi-platform", "buildpack-old-format") + bpPathURI, err = paths.FilePathToURI(destBpPath, "") - prepareDownloadedBuildpackBlobAtURI(t, mockDownloader, destBpPath) - prepareExpectedMultiPlaformImages(t, mockImageFactory, mockImageFetcher, repoName, dist.Target{OS: "linux", Arch: "amd64"}, - expectedMultiPlatformImage{digest: newDigest(t, repoName, "sha256:b9d056b83bb6446fee29e89a7fcf10203c562c1f59586a6e2f39c903597bda34")}) - prepareExpectedMultiPlaformImages(t, mockImageFactory, mockImageFetcher, repoName, dist.Target{OS: "linux", Arch: "arm"}, - expectedMultiPlatformImage{digest: newDigest(t, repoName, "sha256:b9d056b83bb6446fee29e89a7fcf10203c562c1f59586a6e2f39c903597bda35")}) - }) + prepareDownloadedBuildpackBlobAtURI(t, mockDownloader, destBpPath) + prepareExpectedMultiPlaformImages(t, mockImageFactory, mockImageFetcher, repoName, dist.Target{OS: "linux", Arch: "amd64"}, + expectedMultiPlatformImage{digest: newDigest(t, repoName, "sha256:b9d056b83bb6446fee29e89a7fcf10203c562c1f59586a6e2f39c903597bda34")}) + prepareExpectedMultiPlaformImages(t, mockImageFactory, mockImageFetcher, repoName, dist.Target{OS: "linux", Arch: "arm"}, + expectedMultiPlatformImage{digest: newDigest(t, repoName, "sha256:b9d056b83bb6446fee29e89a7fcf10203c562c1f59586a6e2f39c903597bda35")}) + }) - it("creates a multi-platform buildpack and pushes it to a registry", func() { - // Define targets we want to package - targets = []dist.Target{{OS: "linux", Arch: "amd64"}, {OS: "linux", Arch: "arm"}} + it("creates a multi-platform buildpack and pushes it to a registry", func() { + // Define targets we want to package + targets = []dist.Target{{OS: "linux", Arch: "amd64"}, {OS: "linux", Arch: "arm"}} - h.AssertNil(t, subject.PackageBuildpack(context.TODO(), client.PackageBuildpackOptions{ - Format: client.FormatImage, - Publish: true, - RelativeBaseDir: "", - Name: repoName, - Config: pubbldpkg.Config{ - Buildpack: dist.BuildpackURI{URI: bpPathURI}, - Targets: []dist.Target{}, - }, - Targets: targets, - PullPolicy: image.PullNever, - })) + h.AssertNil(t, subject.PackageBuildpack(context.TODO(), client.PackageBuildpackOptions{ + Format: client.FormatImage, + Publish: true, + RelativeBaseDir: "", + Name: repoName, + Config: pubbldpkg.Config{ + Buildpack: dist.BuildpackURI{URI: bpPathURI}, + Targets: []dist.Target{}, + }, + Targets: targets, + PullPolicy: image.PullNever, + })) - // index is not saved locally - h.AssertPathDoesNotExists(t, indexLocalPath) + // index is not saved locally + h.AssertPathDoesNotExists(t, indexLocalPath) - // Push operation was done - h.AssertTrue(t, index.PushCalled) - h.AssertTrue(t, index.PurgeOption) + // Push operation was done + h.AssertTrue(t, index.PushCalled) + h.AssertTrue(t, index.PurgeOption) - // index has the two expected manifests amd64 and arm - indexManifest, err := index.IndexManifest() - h.AssertNil(t, err) - h.AssertEq(t, len(indexManifest.Manifests), 2) - }) + // index has the two expected manifests amd64 and arm + indexManifest, err := index.IndexManifest() + h.AssertNil(t, err) + h.AssertEq(t, len(indexManifest.Manifests), 2) }) }) diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index abb36cbbb..377c2d636 100644 --- a/pkg/image/fetcher.go +++ b/pkg/image/fetcher.go @@ -187,10 +187,9 @@ func (f *Fetcher) fetchRemoteImage(name string, target *dist.Target) (imgutil.Im if target == nil { image, err = remote.NewImage(name, f.keychain, remote.FromBaseImage(name)) } else { - platform := imgutil.Platform{OS: target.OS, Architecture: target.Arch} - // when targeting a registry, we need to use variant if available to hit the correct image - platform.Variant = target.ArchVariant + platform := imgutil.Platform{OS: target.OS, Architecture: target.Arch, Variant: target.ArchVariant} if len(target.Distributions) > 0 { + // We need to set platform distribution information so that it will be reflected in the image config. // We assumed the given target's distributions were expanded, and we are just dealing with 1 version. platform.OSVersion = target.Distributions[0].Version if len(target.Distributions) > 1 {