From 099422aafb2080a51ef3408213f81ee90c9cbeda Mon Sep 17 00:00:00 2001 From: Anthony Emengo Date: Mon, 26 Oct 2020 18:00:32 -0400 Subject: [PATCH 1/3] Read OS platform from config-file during "pack package-buildpack" - unit tests Signed-off-by: Anthony Emengo --- buildpackage/config_reader.go | 5 ++ buildpackage/config_reader_test.go | 26 ++++++++++ create_builder_test.go | 3 ++ internal/commands/package_buildpack.go | 10 ---- internal/commands/package_buildpack_test.go | 56 --------------------- internal/dist/dist.go | 4 ++ package_buildpack.go | 37 +++----------- package_buildpack_test.go | 32 ++++++++++-- 8 files changed, 73 insertions(+), 100 deletions(-) diff --git a/buildpackage/config_reader.go b/buildpackage/config_reader.go index 4d766aa679..d3e07b8380 100644 --- a/buildpackage/config_reader.go +++ b/buildpackage/config_reader.go @@ -16,6 +16,7 @@ import ( type Config struct { Buildpack dist.BuildpackURI `toml:"buildpack"` Dependencies []dist.ImageOrURI `toml:"dependencies"` + Platform dist.Platform `toml:"platform"` } // NewConfigReader returns an instance of ConfigReader. It does not take any parameters. @@ -51,6 +52,10 @@ func (r *ConfigReader) Read(path string) (Config, error) { return packageConfig, errors.Errorf("missing %s configuration", style.Symbol("buildpack.uri")) } + if packageConfig.Platform.OS == "" { + packageConfig.Platform.OS = "linux" + } + configDir, err := filepath.Abs(filepath.Dir(path)) if err != nil { return packageConfig, err diff --git a/buildpackage/config_reader_test.go b/buildpackage/config_reader_test.go index 0bab3b5837..1f4ce76a5c 100644 --- a/buildpackage/config_reader_test.go +++ b/buildpackage/config_reader_test.go @@ -46,11 +46,26 @@ func testBuildpackageConfigReader(t *testing.T, when spec.G, it spec.S) { config, err := packageConfigReader.Read(configFile) h.AssertNil(t, err) + h.AssertEq(t, config.Platform.OS, "some-os") h.AssertEq(t, config.Buildpack.URI, "https://example.com/bp/a.tgz") h.AssertEq(t, len(config.Dependencies), 1) h.AssertEq(t, config.Dependencies[0].URI, "https://example.com/bp/b.tgz") }) + it("returns a config with 'linux' as default when platform is missing", func() { + configFile := filepath.Join(tmpDir, "package.toml") + + err := ioutil.WriteFile(configFile, []byte(validPackageWithoutPlatformToml), os.ModePerm) + h.AssertNil(t, err) + + packageConfigReader := buildpackage.NewConfigReader() + + config, err := packageConfigReader.Read(configFile) + h.AssertNil(t, err) + + h.AssertEq(t, config.Platform.OS, "linux") + }) + it("returns an error when toml decode fails", func() { configFile := filepath.Join(tmpDir, "package.toml") @@ -192,6 +207,17 @@ const validPackageToml = ` [buildpack] uri = "https://example.com/bp/a.tgz" +[[dependencies]] +uri = "https://example.com/bp/b.tgz" + +[platform] +os = "some-os" +` + +const validPackageWithoutPlatformToml = ` +[buildpack] +uri = "https://example.com/bp/a.tgz" + [[dependencies]] uri = "https://example.com/bp/b.tgz" ` diff --git a/create_builder_test.go b/create_builder_test.go index 26905a64f8..6fd54077ab 100644 --- a/create_builder_test.go +++ b/create_builder_test.go @@ -762,6 +762,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: cnbFile, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: buildpackPath}, }, Format: "file", @@ -842,6 +843,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packageImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(bpd)}, }, Publish: true, @@ -915,6 +917,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packageImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(bpd)}, }, Publish: true, diff --git a/internal/commands/package_buildpack.go b/internal/commands/package_buildpack.go index 785e2643c3..7bc84ebc38 100644 --- a/internal/commands/package_buildpack.go +++ b/internal/commands/package_buildpack.go @@ -19,7 +19,6 @@ import ( type PackageBuildpackFlags struct { PackageTomlPath string Format string - OS string Publish bool Policy string } @@ -72,7 +71,6 @@ func PackageBuildpack(logger logging.Logger, cfg config.Config, client Buildpack if err := client.PackageBuildpack(cmd.Context(), pack.PackageBuildpackOptions{ Name: name, Format: flags.Format, - OS: flags.OS, Config: cfg, Publish: flags.Publish, PullPolicy: pullPolicy, @@ -93,10 +91,6 @@ func PackageBuildpack(logger logging.Logger, cfg config.Config, client Buildpack cmd.Flags().StringVarP(&flags.Format, "format", "f", "", `Format to save package as ("image" or "file")`) cmd.Flags().BoolVar(&flags.Publish, "publish", false, `Publish to registry (applies to "--format=image" only)`) - cmd.Flags().StringVar(&flags.OS, "os", "", `Operating system format of the package OCI image: "linux" or "windows" (defaults to "linux", except local images which use the daemon OS)`) - if !cfg.Experimental { - cmd.Flags().MarkHidden("os") - } cmd.Flags().StringVar(&flags.Policy, "pull-policy", "", "Pull policy to use. Accepted values are always, never, and if-not-present. The default is always") AddHelpFlag(cmd, "package-buildpack") @@ -112,9 +106,5 @@ func validatePackageBuildpackFlags(p *PackageBuildpackFlags, cfg config.Config) return errors.Errorf("Please provide a package config path, using --config") } - if p.OS != "" && !cfg.Experimental { - return pack.NewExperimentError("Support for OS flag is currently experimental.") - } - return nil } diff --git a/internal/commands/package_buildpack_test.go b/internal/commands/package_buildpack_test.go index 206ff18dc4..82828ec519 100644 --- a/internal/commands/package_buildpack_test.go +++ b/internal/commands/package_buildpack_test.go @@ -122,34 +122,6 @@ func testPackageBuildpackCommand(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, receivedOptions.PullPolicy, pubcfg.PullAlways) }) }) - - when("--os", func() { - when("experimental enabled", func() { - it("creates package with correct image name and os", func() { - fakeBuildpackPackager := &fakes.FakeBuildpackPackager{} - - packageBuildpackCommand := packageBuildpackCommand( - withBuildpackPackager(fakeBuildpackPackager), - withExperimental(), - ) - - packageBuildpackCommand.SetArgs( - []string{ - "some-image-name", - "--config", "/path/to/some/file", - "--os", "windows", - }, - ) - - err := packageBuildpackCommand.Execute() - h.AssertNil(t, err) - - receivedOptions := fakeBuildpackPackager.CreateCalledWithOptions - - h.AssertEq(t, receivedOptions.OS, "windows") - }) - }) - }) }) }) @@ -250,28 +222,6 @@ func testPackageBuildpackCommand(t *testing.T, when spec.G, it spec.S) { h.AssertError(t, command.Execute(), "parsing pull policy") }) }) - - when("--os flag is specified but experimental isn't set in the config", func() { - it("errors with a descriptive message", func() { - fakeBuildpackPackager := &fakes.FakeBuildpackPackager{} - - packageBuildpackCommand := packageBuildpackCommand( - withBuildpackPackager(fakeBuildpackPackager), - ) - - packageBuildpackCommand.SetArgs( - []string{ - "some-image-name", - "--config", "/path/to/some/file", - "--os", "windows", - }, - ) - - err := packageBuildpackCommand.Execute() - h.AssertNotNil(t, err) - h.AssertError(t, err, "Support for OS flag is currently experimental") - }) - }) }) } @@ -338,12 +288,6 @@ func withPackageConfigPath(path string) packageCommandOption { } } -func withExperimental() packageCommandOption { - return func(config *packageCommandConfig) { - config.clientConfig.Experimental = true - } -} - func whereReadReturns(config pubbldpkg.Config, err error) func(*fakes.FakePackageConfigReader) { return func(r *fakes.FakePackageConfigReader) { r.ReadReturnConfig = config diff --git a/internal/dist/dist.go b/internal/dist/dist.go index ed75b409ae..cb5fcfe5ed 100644 --- a/internal/dist/dist.go +++ b/internal/dist/dist.go @@ -17,6 +17,10 @@ type ImageOrURI struct { ImageRef } +type Platform struct { + OS string `toml:"os"` +} + type Order []OrderEntry type OrderEntry struct { diff --git a/package_buildpack.go b/package_buildpack.go index 6d4e1bd399..0b77eb6edf 100644 --- a/package_buildpack.go +++ b/package_buildpack.go @@ -31,9 +31,6 @@ type PackageBuildpackOptions struct { // Type of output format, The options are the either the const FormatImage, or FormatFile. Format string - // Type of OCI image to generate in the image or file. The options are "windows" or "linux" - OS string - // Defines the Buildpacks configuration. Config pubbldpkg.Config @@ -51,20 +48,11 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti opts.Format = FormatImage } - if opts.OS == "" { - osType, err := c.defaultOSType(ctx, opts.Publish, opts.Format) - if err != nil { - return errors.Wrap(err, "daemon OS cannot be detected") - } - - opts.OS = osType - } - - if opts.OS == "windows" && !c.experimental { + if opts.Config.Platform.OS == "windows" && !c.experimental { return NewExperimentError("Windows buildpackage support is currently experimental.") } - writerFactory, err := layer.NewWriterFactory(opts.OS) + writerFactory, err := layer.NewWriterFactory(opts.Config.Platform.OS) if err != nil { return errors.Wrap(err, "creating layer writer factory") } @@ -144,26 +132,13 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti switch opts.Format { case FormatFile: - return packageBuilder.SaveAsFile(opts.Name, opts.OS) + return packageBuilder.SaveAsFile(opts.Name, opts.Config.Platform.OS) case FormatImage: - _, err = packageBuilder.SaveAsImage(opts.Name, opts.Publish, opts.OS) + //TODO: should check if saving linux image on windows daemon ??? + // + _, err = packageBuilder.SaveAsImage(opts.Name, opts.Publish, opts.Config.Platform.OS) return errors.Wrapf(err, "saving image") default: return errors.Errorf("unknown format: %s", style.Symbol(opts.Format)) } } - -func (c *Client) defaultOSType(ctx context.Context, publish bool, format string) (string, error) { - if publish || format == FormatFile { - c.logger.Warnf(`buildpackage OS unspecified - defaulting to "linux"`) - - return "linux", nil - } - - info, err := c.docker.Info(ctx) - if err != nil { - return "", err - } - - return info.OSType, nil -} diff --git a/package_buildpack_test.go b/package_buildpack_test.go index ef9a48c441..f86f14dd3e 100644 --- a/package_buildpack_test.go +++ b/package_buildpack_test.go @@ -84,6 +84,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err := subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "Fake-Name", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: ""}, }, Publish: true, @@ -100,6 +101,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err := subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "Fake-Name", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: bpURL}, }, Publish: true, @@ -117,6 +119,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err := subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "Fake-Name", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: bpURL}, }, Publish: true, @@ -148,6 +151,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err := subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "test", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{BuildpackURI: dist.BuildpackURI{URI: dependencyPath}}}, }, @@ -186,6 +190,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { Format: pack.FormatImage, Name: fakeImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: daemonOS}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.basic", Version: "2.3.4"}, @@ -203,7 +208,6 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { it("fails without experimental on Windows daemons", func() { windowsMockDockerClient := testmocks.NewMockCommonAPIClient(mockController) - windowsMockDockerClient.EXPECT().Info(context.TODO()).Return(types.Info{OSType: "windows"}, nil).AnyTimes() packClientWithoutExperimental, err := pack.NewClient( pack.WithDockerClient(windowsMockDockerClient), @@ -211,7 +215,13 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { ) h.AssertNil(t, err) - err = packClientWithoutExperimental.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{}) + err = packClientWithoutExperimental.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ + Config: pubbldpkg.Config{ + Platform: dist.Platform{ + OS: "windows", + }, + }, + }) h.AssertError(t, err, "Windows buildpackage support is currently experimental.") }) }) @@ -228,6 +238,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: nestedPackage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.nested", Version: "2.3.4"}, @@ -267,6 +278,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packageImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, @@ -293,6 +305,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packageImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, @@ -319,6 +332,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packageImage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, @@ -344,6 +358,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertError(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "some/package", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, @@ -368,6 +383,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertError(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: "some/package", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, @@ -409,8 +425,8 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, packClientWithExperimental.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Format: pack.FormatFile, Name: packagePath, - OS: imageOS, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: imageOS}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.basic", Version: "2.3.4"}, @@ -466,6 +482,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: nestedPackage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(childDescriptor)}, }, Publish: true, @@ -481,6 +498,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{ImageRef: dist.ImageRef{ImageName: nestedPackage.Name()}}}, }, @@ -500,6 +518,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{BuildpackURI: dist.BuildpackURI{URI: createBuildpack(childDescriptor)}}}, }, @@ -521,6 +540,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err = subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{BuildpackURI: dist.BuildpackURI{URI: bpURL}}}, }, @@ -543,6 +563,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { err = subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{BuildpackURI: dist.BuildpackURI{URI: bpURL}}}, }, @@ -576,6 +597,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: nestedPackage.Name(), Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(childDescriptor)}, }, Publish: true, @@ -591,6 +613,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{ImageRef: dist.ImageRef{ImageName: nestedPackage.Name()}}, {BuildpackURI: dist.BuildpackURI{URI: createBuildpack(secondChildDescriptor)}}}, @@ -614,6 +637,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: dependencyPackagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(childDescriptor)}, }, PullPolicy: pubcfg.PullAlways, @@ -629,6 +653,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ Name: packagePath, Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(packageDescriptor)}, Dependencies: []dist.ImageOrURI{{BuildpackURI: dist.BuildpackURI{URI: dependencyPackagePath}}}, }, @@ -651,6 +676,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { Name: "some-buildpack", Format: "invalid-format", Config: pubbldpkg.Config{ + Platform: dist.Platform{OS: "linux"}, Buildpack: dist.BuildpackURI{URI: createBuildpack(dist.BuildpackDescriptor{ API: api.MustParse("0.2"), Info: dist.BuildpackInfo{ID: "bp.1", Version: "1.2.3"}, From c3e7e8d15180efb8edd7dc07c9a70ad72cc31314 Mon Sep 17 00:00:00 2001 From: Anthony Emengo Date: Tue, 27 Oct 2020 11:57:05 -0400 Subject: [PATCH 2/3] Read OS platform from config-file during "pack package-buildpack" - acceptance tests Signed-off-by: Anthony Emengo --- acceptance/acceptance_test.go | 79 +++++++++++++------ acceptance/buildpacks/manager.go | 7 -- .../buildpacks/package_file_buildpack.go | 9 --- .../buildpacks/package_image_buildpack.go | 9 --- .../testdata/pack_fixtures/package.toml | 5 +- .../pack_fixtures/package_aggregate.toml | 5 +- .../pack_fixtures/package_for_build_cmd.toml | 5 +- 7 files changed, 67 insertions(+), 52 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index c369c528b1..d538d0cdc6 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -235,9 +235,9 @@ func testWithoutSpecificBuilderRequirement( when("package-buildpack", func() { var ( - tmpDir string - simplePackageConfigPath string - buildpackManager buildpacks.BuildpackManager + tmpDir string + buildpackManager buildpacks.BuildpackManager + simplePackageConfigFixtureName = "package.toml" ) it.Before(func() { @@ -250,9 +250,6 @@ func testWithoutSpecificBuilderRequirement( tmpDir, err = ioutil.TempDir("", "package-buildpack-tests") assert.Nil(err) - simplePackageConfigPath = filepath.Join(tmpDir, "package.toml") - h.CopyFile(t, pack.FixtureManager().FixtureLocation("package.toml"), simplePackageConfigPath) - buildpackManager = buildpacks.NewBuildpackManager(t, assert) buildpackManager.PrepareBuildpacks(tmpDir, buildpacks.SimpleLayersParent, buildpacks.SimpleLayers) }) @@ -268,7 +265,7 @@ func testWithoutSpecificBuilderRequirement( } - generateAggregatePackageToml := func(buildpackURI, nestedPackageName string) string { + generateAggregatePackageToml := func(buildpackURI, nestedPackageName, os string) string { t.Helper() packageTomlFile, err := ioutil.TempFile(tmpDir, "package_aggregate-*.toml") assert.Nil(err) @@ -279,6 +276,7 @@ func testWithoutSpecificBuilderRequirement( map[string]interface{}{ "BuildpackURI": buildpackURI, "PackageName": nestedPackageName, + "OS": os, }, ) @@ -290,7 +288,9 @@ func testWithoutSpecificBuilderRequirement( when("no --format is provided", func() { it("creates the package as image", func() { packageName := "test/package-" + h.RandString(10) - output := pack.RunSuccessfully("package-buildpack", packageName, "-c", simplePackageConfigPath) + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) + + output := pack.RunSuccessfully("package-buildpack", packageName, "-c", packageTomlPath) assertions.NewOutputAssertionManager(t, output).ReportsPackageCreation(packageName) defer h.DockerRmi(dockerCli, packageName) @@ -304,7 +304,8 @@ func testWithoutSpecificBuilderRequirement( nestedPackageName := "test/package-" + h.RandString(10) packageName := "test/package-" + h.RandString(10) - aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName) + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) + aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName, dockerHostOS()) packageBuildpack := buildpacks.NewPackageImage( t, @@ -317,7 +318,7 @@ func testWithoutSpecificBuilderRequirement( t, pack, nestedPackageName, - simplePackageConfigPath, + packageTomlPath, buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers), ), ), @@ -333,27 +334,26 @@ func testWithoutSpecificBuilderRequirement( it("publishes image to registry", func() { h.SkipIf(t, !pack.Supports("package-buildpack --os"), "os not supported") + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) nestedPackageName := registryConfig.RepoName("test/package-" + h.RandString(10)) nestedPackage := buildpacks.NewPackageImage( t, pack, nestedPackageName, - simplePackageConfigPath, + packageTomlPath, buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers), buildpacks.WithPublish(), - buildpacks.WithOS(dockerHostOS()), ) buildpackManager.PrepareBuildpacks(tmpDir, nestedPackage) defer h.DockerRmi(dockerCli, nestedPackageName) - aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName) + aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName, dockerHostOS()) packageName := registryConfig.RepoName("test/package-" + h.RandString(10)) output := pack.RunSuccessfully( "package-buildpack", packageName, "-c", aggregatePackageToml, "--publish", - "--os", dockerHostOS(), ) defer h.DockerRmi(dockerCli, packageName) assertions.NewOutputAssertionManager(t, output).ReportsPackagePublished(packageName) @@ -370,17 +370,18 @@ func testWithoutSpecificBuilderRequirement( when("--pull-policy=never", func() { it("should use local image", func() { + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) nestedPackageName := "test/package-" + h.RandString(10) nestedPackage := buildpacks.NewPackageImage( t, pack, nestedPackageName, - simplePackageConfigPath, + packageTomlPath, buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers), ) buildpackManager.PrepareBuildpacks(tmpDir, nestedPackage) defer h.DockerRmi(dockerCli, nestedPackageName) - aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName) + aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName, dockerHostOS()) packageName := registryConfig.RepoName("test/package-" + h.RandString(10)) defer h.DockerRmi(dockerCli, packageName) @@ -396,18 +397,19 @@ func testWithoutSpecificBuilderRequirement( }) it("should not pull image from registry", func() { + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) nestedPackageName := registryConfig.RepoName("test/package-" + h.RandString(10)) nestedPackage := buildpacks.NewPackageImage( t, pack, nestedPackageName, - simplePackageConfigPath, + packageTomlPath, buildpacks.WithPublish(), buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers), ) buildpackManager.PrepareBuildpacks(tmpDir, nestedPackage) defer h.DockerRmi(dockerCli, nestedPackageName) - aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName) + aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName, dockerHostOS()) packageName := registryConfig.RepoName("test/package-" + h.RandString(10)) defer h.DockerRmi(dockerCli, packageName) @@ -428,11 +430,12 @@ func testWithoutSpecificBuilderRequirement( }) it("creates the package", func() { + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, dockerHostOS()) destinationFile := filepath.Join(tmpDir, "package.cnb") output, err := pack.Run( "package-buildpack", destinationFile, "--format", "file", - "-c", simplePackageConfigPath, + "-c", packageTomlPath, ) assert.Nil(err) assertions.NewOutputAssertionManager(t, output).ReportsPackageCreation(destinationFile) @@ -538,16 +541,17 @@ func testWithoutSpecificBuilderRequirement( fmt.Sprintf("buildpack-%s.cnb", h.RandString(8)), ) + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, "package_for_build_cmd.toml", dockerHostOS()) + packageFile := buildpacks.NewPackageFile( t, pack, packageFileLocation, - pack.FixtureManager().FixtureLocation("package_for_build_cmd.toml"), + packageTomlPath, buildpacks.WithRequiredBuildpacks( buildpacks.FolderSimpleLayersParent, buildpacks.FolderSimpleLayers, ), - buildpacks.WithOS(dockerHostOS()), ) buildpackManager.PrepareBuildpacks(tmpDir, packageFile) @@ -570,13 +574,14 @@ func testWithoutSpecificBuilderRequirement( when("buildpack image", func() { when("inspect-buildpack", func() { it("succeeds", func() { + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, "package_for_build_cmd.toml", dockerHostOS()) packageImageName := registryConfig.RepoName("buildpack-" + h.RandString(8)) packageImage := buildpacks.NewPackageImage( t, pack, packageImageName, - pack.FixtureManager().FixtureLocation("package_for_build_cmd.toml"), + packageTomlPath, buildpacks.WithRequiredBuildpacks( buildpacks.FolderSimpleLayersParent, buildpacks.FolderSimpleLayers, @@ -1327,7 +1332,6 @@ func testAcceptance( buildpacks.FolderSimpleLayersParent, buildpacks.FolderSimpleLayers, ), - buildpacks.WithOS(dockerHostOS()), ) buildpackManager.PrepareBuildpacks(tmpDir, packageFile) @@ -2389,13 +2393,14 @@ func createBuilder( buildpacks.ReadEnv, } + packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, "package.toml", dockerHostOS()) packageImageName := registryConfig.RepoName("simple-layers-package-image-buildpack-" + h.RandString(8)) packageImageBuildpack := buildpacks.NewPackageImage( t, pack, packageImageName, - pack.FixtureManager().FixtureLocation("package.toml"), + packageTomlPath, buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers), ) @@ -2449,6 +2454,32 @@ func createBuilder( return bldr, nil } +func generatePackageTomlWithOS( + t *testing.T, + assert h.AssertionManager, + pack *invoke.PackInvoker, + tmpDir string, + fixtureName string, + platform_os string, +) string { + t.Helper() + + packageTomlFile, err := ioutil.TempFile(tmpDir, "package-*.toml") + assert.Nil(err) + + pack.FixtureManager().TemplateFixtureToFile( + fixtureName, + packageTomlFile, + map[string]interface{}{ + "OS": platform_os, + }, + ) + + assert.Nil(packageTomlFile.Close()) + + return packageTomlFile.Name() +} + func createStack(t *testing.T, dockerCli client.CommonAPIClient, runImageMirror string) error { t.Helper() t.Log("creating stack images...") diff --git a/acceptance/buildpacks/manager.go b/acceptance/buildpacks/manager.go index f3777bd42e..5987155c93 100644 --- a/acceptance/buildpacks/manager.go +++ b/acceptance/buildpacks/manager.go @@ -53,7 +53,6 @@ func (b BuildpackManager) PrepareBuildpacks(destination string, buildpacks ...Te } type Modifiable interface { - SetOS(string) SetPublish() SetBuildpacks([]TestBuildpack) } @@ -70,9 +69,3 @@ func WithPublish() PackageModifier { p.SetPublish() } } - -func WithOS(osVal string) PackageModifier { - return func(p Modifiable) { - p.SetOS(osVal) - } -} diff --git a/acceptance/buildpacks/package_file_buildpack.go b/acceptance/buildpacks/package_file_buildpack.go index 3b6d5cb616..ab1694524b 100644 --- a/acceptance/buildpacks/package_file_buildpack.go +++ b/acceptance/buildpacks/package_file_buildpack.go @@ -21,11 +21,6 @@ type PackageFile struct { destination string sourceConfigLocation string buildpacks []TestBuildpack - os string -} - -func (p *PackageFile) SetOS(os string) { - p.os = os } func (p *PackageFile) SetBuildpacks(buildpacks []TestBuildpack) { @@ -81,10 +76,6 @@ func (p PackageFile) Prepare(sourceDir, _ string) error { "--format", "file", } - if p.os != "" { - packArgs = append(packArgs, "--os", p.os) - } - output := p.pack.RunSuccessfully("package-buildpack", packArgs...) if !strings.Contains(output, fmt.Sprintf("Successfully created package '%s'", p.destination)) { diff --git a/acceptance/buildpacks/package_image_buildpack.go b/acceptance/buildpacks/package_image_buildpack.go index fc616176a9..ddd882b548 100644 --- a/acceptance/buildpacks/package_image_buildpack.go +++ b/acceptance/buildpacks/package_image_buildpack.go @@ -23,11 +23,6 @@ type PackageImage struct { sourceConfigLocation string buildpacks []TestBuildpack publish bool - os string -} - -func (p *PackageImage) SetOS(os string) { - p.os = os } func (p *PackageImage) SetBuildpacks(buildpacks []TestBuildpack) { @@ -88,10 +83,6 @@ func (p PackageImage) Prepare(sourceDir, _ string) error { packArgs = append(packArgs, "--publish") } - if p.os != "" { - packArgs = append(packArgs, "--os", p.os) - } - output := p.pack.RunSuccessfully("package-buildpack", packArgs...) assertOutput := assertions.NewOutputAssertionManager(p.testObject, output) diff --git a/acceptance/testdata/pack_fixtures/package.toml b/acceptance/testdata/pack_fixtures/package.toml index 28694e6601..6368266c38 100644 --- a/acceptance/testdata/pack_fixtures/package.toml +++ b/acceptance/testdata/pack_fixtures/package.toml @@ -1,2 +1,5 @@ [buildpack] -uri = "simple-layers-buildpack.tgz" \ No newline at end of file +uri = "simple-layers-buildpack.tgz" + +[platform] +os = "{{ .OS }}" \ No newline at end of file diff --git a/acceptance/testdata/pack_fixtures/package_aggregate.toml b/acceptance/testdata/pack_fixtures/package_aggregate.toml index 096dc5afb5..e6a5e2c980 100644 --- a/acceptance/testdata/pack_fixtures/package_aggregate.toml +++ b/acceptance/testdata/pack_fixtures/package_aggregate.toml @@ -2,4 +2,7 @@ uri = "{{ .BuildpackURI }}" [[dependencies]] -image = "{{ .PackageName }}" \ No newline at end of file +image = "{{ .PackageName }}" + +[platform] +os = "{{ .OS }}" \ No newline at end of file diff --git a/acceptance/testdata/pack_fixtures/package_for_build_cmd.toml b/acceptance/testdata/pack_fixtures/package_for_build_cmd.toml index 28defc25be..09676a90e0 100644 --- a/acceptance/testdata/pack_fixtures/package_for_build_cmd.toml +++ b/acceptance/testdata/pack_fixtures/package_for_build_cmd.toml @@ -2,4 +2,7 @@ uri = "simple-layers-parent-buildpack" [[dependencies]] -uri = "simple-layers-buildpack" \ No newline at end of file +uri = "simple-layers-buildpack" + +[platform] +os = "{{ .OS }}" \ No newline at end of file From 03c483e46aef4017909dfef96b7c269de2376920 Mon Sep 17 00:00:00 2001 From: Anthony Emengo Date: Wed, 28 Oct 2020 14:51:10 -0400 Subject: [PATCH 3/3] Additional validation - Validate only ['linux', 'windows'] is allowed in platform.os of package.toml - Validate correct platform.os for DOCKER_OS Signed-off-by: Anthony Emengo --- buildpackage/config_reader.go | 5 +++++ buildpackage/config_reader_test.go | 26 ++++++++++++++++++++++++-- package_buildpack.go | 24 ++++++++++++++++++++++-- package_buildpack_test.go | 21 +++++++++++++++++++++ 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/buildpackage/config_reader.go b/buildpackage/config_reader.go index d3e07b8380..653a71f92d 100644 --- a/buildpackage/config_reader.go +++ b/buildpackage/config_reader.go @@ -56,6 +56,11 @@ func (r *ConfigReader) Read(path string) (Config, error) { packageConfig.Platform.OS = "linux" } + if packageConfig.Platform.OS != "linux" && packageConfig.Platform.OS != "windows" { + return packageConfig, errors.Errorf("invalid %s configuration: only [%s, %s] is permitted", + style.Symbol("platform.os"), style.Symbol("linux"), style.Symbol("windows")) + } + configDir, err := filepath.Abs(filepath.Dir(path)) if err != nil { return packageConfig, err diff --git a/buildpackage/config_reader_test.go b/buildpackage/config_reader_test.go index 1f4ce76a5c..6af32fadca 100644 --- a/buildpackage/config_reader_test.go +++ b/buildpackage/config_reader_test.go @@ -46,7 +46,7 @@ func testBuildpackageConfigReader(t *testing.T, when spec.G, it spec.S) { config, err := packageConfigReader.Read(configFile) h.AssertNil(t, err) - h.AssertEq(t, config.Platform.OS, "some-os") + h.AssertEq(t, config.Platform.OS, "windows") h.AssertEq(t, config.Buildpack.URI, "https://example.com/bp/a.tgz") h.AssertEq(t, len(config.Dependencies), 1) h.AssertEq(t, config.Dependencies[0].URI, "https://example.com/bp/b.tgz") @@ -114,6 +114,20 @@ func testBuildpackageConfigReader(t *testing.T, when spec.G, it spec.S) { h.AssertError(t, err, "https@@@@@@://example.com/bp/a.tgz") }) + it("returns an error when platform os is invalid", func() { + configFile := filepath.Join(tmpDir, "package.toml") + + err := ioutil.WriteFile(configFile, []byte(invalidPlatformOSPackageToml), os.ModePerm) + h.AssertNil(t, err) + + packageConfigReader := buildpackage.NewConfigReader() + + _, err = packageConfigReader.Read(configFile) + h.AssertNotNil(t, err) + h.AssertError(t, err, "invalid 'platform.os' configuration") + h.AssertError(t, err, "only ['linux', 'windows'] is permitted") + }) + it("returns an error when dependency uri is invalid", func() { configFile := filepath.Join(tmpDir, "package.toml") @@ -211,7 +225,7 @@ uri = "https://example.com/bp/a.tgz" uri = "https://example.com/bp/b.tgz" [platform] -os = "some-os" +os = "windows" ` const validPackageWithoutPlatformToml = ` @@ -259,6 +273,14 @@ uri = "noop-buildpack.tgz" image = "some/package-dep" ` +const invalidPlatformOSPackageToml = ` +[buildpack] +uri = "https://example.com/bp/a.tgz" + +[platform] +os = "some-incorrect-platform" +` + const unknownBPKeyPackageToml = ` [buildpack] url = "noop-buildpack.tgz" diff --git a/package_buildpack.go b/package_buildpack.go index 0b77eb6edf..078cab4d66 100644 --- a/package_buildpack.go +++ b/package_buildpack.go @@ -52,6 +52,11 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti return NewExperimentError("Windows buildpackage support is currently experimental.") } + err := c.validateOSPlatform(ctx, opts.Config.Platform.OS, opts.Publish, opts.Format) + if err != nil { + return err + } + writerFactory, err := layer.NewWriterFactory(opts.Config.Platform.OS) if err != nil { return errors.Wrap(err, "creating layer writer factory") @@ -134,11 +139,26 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti case FormatFile: return packageBuilder.SaveAsFile(opts.Name, opts.Config.Platform.OS) case FormatImage: - //TODO: should check if saving linux image on windows daemon ??? - // _, err = packageBuilder.SaveAsImage(opts.Name, opts.Publish, opts.Config.Platform.OS) return errors.Wrapf(err, "saving image") default: return errors.Errorf("unknown format: %s", style.Symbol(opts.Format)) } } + +func (c *Client) validateOSPlatform(ctx context.Context, os string, publish bool, format string) error { + if publish || format == FormatFile { + return nil + } + + info, err := c.docker.Info(ctx) + if err != nil { + return err + } + + if info.OSType != os { + return errors.Errorf("invalid %s specified: DOCKER_OS is %s", style.Symbol("platform.os"), style.Symbol(info.OSType)) + } + + return nil +} diff --git a/package_buildpack_test.go b/package_buildpack_test.go index f86f14dd3e..f2d2d42ba6 100644 --- a/package_buildpack_test.go +++ b/package_buildpack_test.go @@ -224,6 +224,27 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) { }) h.AssertError(t, err, "Windows buildpackage support is currently experimental.") }) + + it("fails for mismatched platform and daemon os", func() { + windowsMockDockerClient := testmocks.NewMockCommonAPIClient(mockController) + windowsMockDockerClient.EXPECT().Info(context.TODO()).Return(types.Info{OSType: "windows"}, nil).AnyTimes() + + packClientWithoutExperimental, err := pack.NewClient( + pack.WithDockerClient(windowsMockDockerClient), + pack.WithExperimental(false), + ) + h.AssertNil(t, err) + + err = packClientWithoutExperimental.PackageBuildpack(context.TODO(), pack.PackageBuildpackOptions{ + Config: pubbldpkg.Config{ + Platform: dist.Platform{ + OS: "linux", + }, + }, + }) + + h.AssertError(t, err, "invalid 'platform.os' specified: DOCKER_OS is 'windows'") + }) }) when("nested package lives in registry", func() {