Skip to content
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

added targets flag for buildpack new cli #1921

Merged
merged 14 commits into from
Oct 17, 2023
26 changes: 23 additions & 3 deletions internal/commands/buildpack_new.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,25 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"

"github.com/spf13/cobra"

"github.com/buildpacks/pack/internal/style"
"github.com/buildpacks/pack/internal/target"
"github.com/buildpacks/pack/pkg/client"
"github.com/buildpacks/pack/pkg/dist"
"github.com/buildpacks/pack/pkg/logging"
)

// BuildpackNewFlags define flags provided to the BuildpackNew command
type BuildpackNewFlags struct {
API string
Path string
API string
Path string
// Deprecated: Stacks are deprecated
Stacks []string
Targets []string
Version string
}

Expand Down Expand Up @@ -66,11 +70,20 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman
})
}

targets, warn, err := target.ParseTargets(flags.Targets)
WYGIN marked this conversation as resolved.
Show resolved Hide resolved
for _, w := range warn.Messages {
logger.Warn(w)
}
if err != nil {
return err
}

if err := creator.NewBuildpack(cmd.Context(), client.NewBuildpackOptions{
API: flags.API,
ID: id,
Path: path,
Stacks: stacks,
Targets: targets,
Version: flags.Version,
}); err != nil {
return err
Expand All @@ -84,7 +97,14 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman
cmd.Flags().StringVarP(&flags.API, "api", "a", "0.8", "Buildpack API compatibility of the generated buildpack")
cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack")
cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack")
cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{"io.buildpacks.stacks.jammy"}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack"))
cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack"))
cmd.Flags().MarkDeprecated("stacks", "prefer `--targets` instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md")
cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{runtime.GOOS + "/" + runtime.GOARCH},
`Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion]
- Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"'
- case for distribution version: '--targets "windows/amd64:[email protected]"'
- case for different architecture with distributed versions : '--targets "linux/arm/v6:[email protected]" --targets "linux/arm/v6:[email protected]"'
`)

AddHelpFlag(cmd, "new")
return cmd
Expand Down
127 changes: 123 additions & 4 deletions internal/commands/buildpack_new_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"os"
"path/filepath"
"runtime"
"testing"

"github.com/buildpacks/pack/pkg/client"
Expand Down Expand Up @@ -36,6 +37,10 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) {
mockClient *testmocks.MockPackClient
tmpDir string
)
targets := []dist.Target{{
OS: runtime.GOOS,
Arch: runtime.GOARCH,
}}

it.Before(func() {
var err error
Expand All @@ -60,10 +65,7 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) {
ID: "example/some-cnb",
Path: filepath.Join(tmpDir, "some-cnb"),
Version: "1.0.0",
Stacks: []dist.Stack{{
ID: "io.buildpacks.stacks.jammy",
Mixins: []string{},
}},
Targets: targets,
}).Return(nil).MaxTimes(1)

path := filepath.Join(tmpDir, "some-cnb")
Expand All @@ -82,5 +84,122 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) {
h.AssertNotNil(t, err)
h.AssertContains(t, outBuf.String(), "ERROR: directory")
})

when("target flag is specified, ", func() {
it("it uses target to generate artifacts", func() {
mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{
API: "0.8",
ID: "example/targets",
Path: filepath.Join(tmpDir, "targets"),
Version: "1.0.0",
Targets: []dist.Target{{
OS: "linux",
Arch: "arm",
ArchVariant: "v6",
Distributions: []dist.Distribution{{
Name: "ubuntu",
Versions: []string{"14.04", "16.04"},
}},
}},
}).Return(nil).MaxTimes(1)

path := filepath.Join(tmpDir, "targets")
command.SetArgs([]string{"--path", path, "example/targets", "--targets", "linux/arm/v6:[email protected]@16.04"})

err := command.Execute()
h.AssertNil(t, err)
})
it("it should show error when invalid [os]/[arch] passed", func() {
mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{
API: "0.8",
ID: "example/targets",
Path: filepath.Join(tmpDir, "targets"),
Version: "1.0.0",
Targets: []dist.Target{{
OS: "os",
Arch: "arm",
ArchVariant: "v6",
Distributions: []dist.Distribution{{
Name: "ubuntu",
Versions: []string{"14.04", "16.04"},
}},
}},
}).Return(nil).MaxTimes(1)

path := filepath.Join(tmpDir, "targets")
command.SetArgs([]string{"--path", path, "example/targets", "--targets", "os/arm/v6:[email protected]@16.04"})

err := command.Execute()
h.AssertNotNil(t, err)
})
when("it should", func() {
it("support format [os][/arch][/variant]:[name@version@version2];[some-name@version@version2]", func() {
mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{
API: "0.8",
ID: "example/targets",
Path: filepath.Join(tmpDir, "targets"),
Version: "1.0.0",
Targets: []dist.Target{
{
OS: "linux",
Arch: "arm",
ArchVariant: "v6",
Distributions: []dist.Distribution{
{
Name: "ubuntu",
Versions: []string{"14.04", "16.04"},
},
{
Name: "debian",
Versions: []string{"8.10", "10.9"},
},
},
},
{
OS: "windows",
Arch: "amd64",
Distributions: []dist.Distribution{
{
Name: "windows-nano",
Versions: []string{"10.0.19041.1415"},
},
},
},
},
}).Return(nil).MaxTimes(1)

path := filepath.Join(tmpDir, "targets")
command.SetArgs([]string{"--path", path, "example/targets", "--targets", "linux/arm/v6:[email protected]@16.04;[email protected]@10.9", "-t", "windows/amd64:[email protected]"})

err := command.Execute()
h.AssertNil(t, err)
})
})
when("stacks ", func() {
it("flag should show deprecated message when used", func() {
mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{
API: "0.8",
ID: "example/stacks",
Path: filepath.Join(tmpDir, "stacks"),
Version: "1.0.0",
Stacks: []dist.Stack{{
ID: "io.buildpacks.stacks.jammy",
Mixins: []string{},
}},
Targets: targets,
Copy link
Member

@jjbustamante jjbustamante Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my comment here, I will remove this Targets: targets.

  • IF users specifies only stacks, I don't want targets to be added in buildpack.toml, the warning message is great and we will keep the current behavior of pack.
  • IF users specifies stacks and targets then both are allow for now, until stacks is actually deprecated. Based on this slack thread

}).Return(nil).MaxTimes(1)

path := filepath.Join(tmpDir, "stacks")
output := new(bytes.Buffer)
command.SetOut(output)
command.SetErr(output)
command.SetArgs([]string{"--path", path, "example/stacks", "--stacks", "io.buildpacks.stacks.jammy"})

err := command.Execute()
h.AssertNil(t, err)
h.AssertContains(t, output.String(), "Flag --stacks has been deprecated,")
})
})
})
})
}
107 changes: 107 additions & 0 deletions internal/target/parse.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package target

import (
"fmt"
"strings"

"github.com/pkg/errors"

"github.com/buildpacks/pack/internal/style"
"github.com/buildpacks/pack/internal/warn"
"github.com/buildpacks/pack/pkg/dist"
)

func ParseTargets(t []string) (targets []dist.Target, warn warn.Warn, err error) {
for _, v := range t {
target, w, err := ParseTarget(v)
warn.AddWarn(w)
if err != nil {
return nil, warn, err
}
targets = append(targets, target)
}
return targets, warn, nil
}

func ParseTarget(t string) (output dist.Target, warn warn.Warn, err error) {
nonDistro, distros, w, err := getTarget(t)
warn.AddWarn(w)
if v, _ := getSliceAt[string](nonDistro, 0); len(nonDistro) <= 1 && v == "" {
warn.Add(style.Error("os/arch must be defined"))
}
if err != nil {
return output, warn, err
}

Check warning on line 34 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L33-L34

Added lines #L33 - L34 were not covered by tests
os, arch, variant, w, err := getPlatform(nonDistro)
warn.AddWarn(w)
if err != nil {
return output, warn, err
}
v, w, err := ParseDistros(distros)
warn.AddWarn(w)
if err != nil {
return output, warn, err
}

Check warning on line 44 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L43-L44

Added lines #L43 - L44 were not covered by tests
output = dist.Target{
OS: os,
Arch: arch,
ArchVariant: variant,
Distributions: v,
}
return output, warn, err
}

func ParseDistros(distroSlice string) (distros []dist.Distribution, warn warn.Warn, err error) {
distro := strings.Split(distroSlice, ";")
if l := len(distro); l == 1 && distro[0] == "" {
return nil, warn, err
}
for _, d := range distro {
v, w, err := ParseDistro(d)
warn.AddWarn(w)
if err != nil {
return nil, warn, err
}

Check warning on line 64 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L63-L64

Added lines #L63 - L64 were not covered by tests
distros = append(distros, v)
}
return distros, warn, nil
}

func ParseDistro(distroString string) (distro dist.Distribution, warn warn.Warn, err error) {
d := strings.Split(distroString, "@")
if d[0] == "" || len(d) == 0 {
return distro, warn, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@")))
}

Check warning on line 74 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L73-L74

Added lines #L73 - L74 were not covered by tests
if len(d) <= 2 && d[0] == "" {
warn.Add(fmt.Sprintf("distro with name %s has no specific version!", style.Symbol(d[0])))
}

Check warning on line 77 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L76-L77

Added lines #L76 - L77 were not covered by tests
distro.Name = d[0]
distro.Versions = d[1:]
return distro, warn, err
}

func getTarget(t string) (nonDistro []string, distros string, warn warn.Warn, err error) {
target := strings.Split(t, ":")
if _, err := getSliceAt[string](target, 0); err != nil {
return nonDistro, distros, warn, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t)
}

Check warning on line 87 in internal/target/parse.go

View check run for this annotation

Codecov / codecov/patch

internal/target/parse.go#L86-L87

Added lines #L86 - L87 were not covered by tests
if len(target) == 2 && target[0] == "" {
v, _ := getSliceAt[string](target, 1)
warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", v))
} else {
i, _ := getSliceAt[string](target, 0)
nonDistro = strings.Split(i, "/")
}
if i, err := getSliceAt[string](target, 1); err == nil {
distros = i
}
return nonDistro, distros, warn, err
}

func getSliceAt[T interface{}](slice []T, index int) (value T, err error) {
if index < 0 || index >= len(slice) {
return value, errors.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice))
}

return slice[index], err
}
Loading
Loading