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

pack builder create should accept builder env config #1926

Merged
merged 11 commits into from
Oct 31, 2023
22 changes: 21 additions & 1 deletion builder/config_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,27 @@ type RunImageConfig struct {

// BuildConfig build image configuration
type BuildConfig struct {
Image string `toml:"image"`
Image string `toml:"image"`
Env []BuildConfigEnv `toml:"env"`
}

type Suffix string

var SuffixSlice []Suffix = []Suffix{NONE, DEFAULT, OVERRIDE, APPEND, PREPEND}
WYGIN marked this conversation as resolved.
Show resolved Hide resolved

const (
NONE Suffix = ""
DEFAULT Suffix = "default"
OVERRIDE Suffix = "override"
APPEND Suffix = "append"
PREPEND Suffix = "prepend"
)

type BuildConfigEnv struct {
Name string `toml:"name"`
Value string `toml:"value"`
Suffix Suffix `toml:"suffix,omitempty"`
Delim string `toml:"delim,omitempty"`
}

// ReadConfig reads a builder configuration from the file path provided and returns the
Expand Down
58 changes: 57 additions & 1 deletion internal/builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
lifecycleplatform "github.com/buildpacks/lifecycle/platform"
)

var buildConfigDir = cnbBuildConfigDir()

const (
packName = "Pack CLI"

Expand Down Expand Up @@ -67,6 +69,7 @@
// Builder represents a pack builder, used to build images
type Builder struct {
baseImageName string
buildConfigEnv map[string]string
image imgutil.Image
layerWriterFactory archive.TarWriterFactory
lifecycle Lifecycle
Expand Down Expand Up @@ -146,6 +149,7 @@
metadata: metadata,
lifecycleDescriptor: constructLifecycleDescriptor(metadata),
env: map[string]string{},
buildConfigEnv: map[string]string{},
validateMixins: true,
additionalBuildpacks: *buildpack.NewModuleManager(opts.flatten, opts.depth),
additionalExtensions: *buildpack.NewModuleManager(opts.flatten, opts.depth),
Expand Down Expand Up @@ -349,6 +353,11 @@
b.env = env
}

// SetBuildConfigEnv sets an environment variable to a value that will take action on platform environment variables basedon filename suffix
func (b *Builder) SetBuildConfigEnv(env map[string]string) {
b.buildConfigEnv = env
}

// SetOrder sets the order of the builder
func (b *Builder) SetOrder(order dist.Order) {
b.order = order
Expand Down Expand Up @@ -525,6 +534,18 @@
return errors.Wrap(err, "adding run.tar layer")
}

if len(b.buildConfigEnv) > 0 {
logger.Debugf("Provided Build Config Environment Variables\n %s", style.Map(b.env, " ", "\n"))
WYGIN marked this conversation as resolved.
Show resolved Hide resolved
buildConfigEnvTar, err := b.buildConfigEnvLayer(tmpDir, b.buildConfigEnv)
if err != nil {
return errors.Wrap(err, "retrieving build-config-env layer")
}

Check warning on line 542 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L541-L542

Added lines #L541 - L542 were not covered by tests

if err := b.image.AddLayer(buildConfigEnvTar); err != nil {
return errors.Wrap(err, "adding build-config-env layer")
}

Check warning on line 546 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L545-L546

Added lines #L545 - L546 were not covered by tests
}

if len(b.env) > 0 {
logger.Debugf("Provided Environment Variables\n %s", style.Map(b.env, " ", "\n"))
}
Expand Down Expand Up @@ -903,7 +924,7 @@
}

// can't use filepath.Join(), to ensure Windows doesn't transform it to Windows join
for _, path := range []string{cnbDir, dist.BuildpacksDir, dist.ExtensionsDir, platformDir, platformDir + "/env"} {
for _, path := range []string{cnbDir, dist.BuildpacksDir, dist.ExtensionsDir, platformDir, platformDir + "/env", buildConfigDir, buildConfigDir + "/env"} {
if err := lw.WriteHeader(b.rootOwnedDir(path, ts)); err != nil {
return "", errors.Wrapf(err, "creating %s dir in layer", style.Symbol(path))
}
Expand Down Expand Up @@ -1102,6 +1123,33 @@
return fh.Name(), nil
}

func (b *Builder) buildConfigEnvLayer(dest string, env map[string]string) (string, error) {
fh, err := os.Create(filepath.Join(dest, "build-config-env.tar"))
if err != nil {
return "", err
}

Check warning on line 1130 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L1129-L1130

Added lines #L1129 - L1130 were not covered by tests
defer fh.Close()

lw := b.layerWriterFactory.NewWriter(fh)
defer lw.Close()

for k, v := range env {
if err := lw.WriteHeader(&tar.Header{
Name: path.Join(buildConfigDir, "env", k),
Size: int64(len(v)),
Mode: 0644,
ModTime: archive.NormalizedDateTime,
}); err != nil {
return "", err
}

Check warning on line 1144 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L1143-L1144

Added lines #L1143 - L1144 were not covered by tests
if _, err := lw.Write([]byte(v)); err != nil {
return "", err
}

Check warning on line 1147 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L1146-L1147

Added lines #L1146 - L1147 were not covered by tests
}

return fh.Name(), nil
}

func (b *Builder) whiteoutLayer(tmpDir string, i int, bpInfo dist.ModuleInfo) (string, error) {
bpWhiteoutsTmpDir := filepath.Join(tmpDir, strconv.Itoa(i)+"_whiteouts")
if err := os.MkdirAll(bpWhiteoutsTmpDir, os.ModePerm); err != nil {
Expand Down Expand Up @@ -1257,3 +1305,11 @@
func (e errModuleTar) Path() string {
return ""
}

func cnbBuildConfigDir() string {
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this allows the pack user to specify the location of the build config directory within the builder by setting an environment variable in pack's environment. We might want to add some text to pack builder create --help so that this option is discoverable. Or maybe we could add it as a field in builder.toml ...or just leave it non-configurable. WDYT @WYGIN @jjbustamante?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am not sure about adding pack builder create --help, IMO the pack builder create --help is about giving better context to user about the list of commands available for the end-user to use, and i think CNB_BUILD_CONFIG_DIR in cli help might not required
so i added it to the docs where builder.toml fields and tables are specified

if env, ok := os.LookupEnv("CNB_BUILD_CONFIG_DIR"); !ok {
return "/cnb/build-config"
} else {
return env
}

Check warning on line 1314 in internal/builder/builder.go

View check run for this annotation

Codecov / codecov/patch

internal/builder/builder.go#L1313-L1314

Added lines #L1313 - L1314 were not covered by tests
}
43 changes: 43 additions & 0 deletions internal/builder/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,20 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
)
})

it("creates the build-config dir", func() {
h.AssertNil(t, subject.Save(logger, builder.CreatorMetadata{}))
h.AssertEq(t, baseImage.IsSaved(), true)

layerTar, err := baseImage.FindLayerWithPath("/cnb/build-config")
h.AssertNil(t, err)
h.AssertOnTarEntry(t, layerTar, "/cnb/build-config",
h.IsDirectory(),
h.HasOwnerAndGroup(0, 0),
WYGIN marked this conversation as resolved.
Show resolved Hide resolved
h.HasFileMode(0755),
h.HasModTime(archive.NormalizedDateTime),
)
})

it("creates the buildpacks dir", func() {
h.AssertNil(t, subject.Save(logger, builder.CreatorMetadata{}))
h.AssertEq(t, baseImage.IsSaved(), true)
Expand Down Expand Up @@ -1607,6 +1621,35 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
})
})

when("#SetBuildConfigEnv", func() {
it.Before(func() {
subject.SetBuildConfigEnv(map[string]string{
"SOME_KEY": "some-val",
"OTHER_KEY.append": "other-val",
"OTHER_KEY.delim": ":",
})
h.AssertNil(t, subject.Save(logger, builder.CreatorMetadata{}))
h.AssertEq(t, baseImage.IsSaved(), true)
})

it("adds the env vars as files to the image", func() {
layerTar, err := baseImage.FindLayerWithPath("/cnb/build-config/env/SOME_KEY")
h.AssertNil(t, err)
h.AssertOnTarEntry(t, layerTar, "/cnb/build-config/env/SOME_KEY",
h.ContentEquals(`some-val`),
h.HasModTime(archive.NormalizedDateTime),
)
h.AssertOnTarEntry(t, layerTar, "/cnb/build-config/env/OTHER_KEY.append",
h.ContentEquals(`other-val`),
h.HasModTime(archive.NormalizedDateTime),
)
h.AssertOnTarEntry(t, layerTar, "/cnb/build-config/env/OTHER_KEY.delim",
h.ContentEquals(`:`),
h.HasModTime(archive.NormalizedDateTime),
)
})
})

when("#SetEnv", func() {
it.Before(func() {
subject.SetEnv(map[string]string{
Expand Down
98 changes: 98 additions & 0 deletions internal/commands/builder_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,18 @@
return errors.Wrap(err, "getting absolute path for config")
}

envMap, warnings, err := ParseBuildConfigEnv(builderConfig.Build.Env, flags.BuilderTomlPath)
for _, v := range warnings {
logger.Warn(v)
}

Check warning on line 81 in internal/commands/builder_create.go

View check run for this annotation

Codecov / codecov/patch

internal/commands/builder_create.go#L80-L81

Added lines #L80 - L81 were not covered by tests
if err != nil {
return err
}

Check warning on line 84 in internal/commands/builder_create.go

View check run for this annotation

Codecov / codecov/patch

internal/commands/builder_create.go#L83-L84

Added lines #L83 - L84 were not covered by tests

imageName := args[0]
if err := pack.CreateBuilder(cmd.Context(), client.CreateBuilderOptions{
RelativeBaseDir: relativeBaseDir,
BuildConfigEnv: envMap,
BuilderName: imageName,
Config: builderConfig,
Publish: flags.Publish,
Expand Down Expand Up @@ -137,3 +146,92 @@

return nil
}

func getActionType(suffix builder.Suffix) (suffixString string, err error) {
const delim = "."
switch suffix {
case builder.NONE:
return "", nil
case builder.DEFAULT:
return delim + string(builder.DEFAULT), nil
case builder.OVERRIDE:
return delim + string(builder.OVERRIDE), nil
case builder.APPEND:
return delim + string(builder.APPEND), nil
case builder.PREPEND:
return delim + string(builder.PREPEND), nil
default:
return suffixString, errors.Errorf("unknown action type %s", style.Symbol(string(suffix)))
}
}

func getFilePrefixSuffix(filename string) (prefix, suffix string, err error) {
val := strings.Split(filename, ".")
if len(val) <= 1 {
return val[0], suffix, errors.Errorf("Suffix might be null")
}
if len(val) == 2 {
suffix = val[1]
} else {
strings.Join(val[1:], ".")
}

Check warning on line 177 in internal/commands/builder_create.go

View check run for this annotation

Codecov / codecov/patch

internal/commands/builder_create.go#L176-L177

Added lines #L176 - L177 were not covered by tests
return val[0], suffix, err
}

func getBuildConfigEnvFileName(env builder.BuildConfigEnv) (suffixName, delimName string, err error) {
suffix, err := getActionType(env.Suffix)
if err != nil {
return suffixName, delimName, err
}
if suffix == "" || len(suffix) == 0 {
suffixName = env.Name
} else {
suffixName = env.Name + suffix
}
if delim := env.Delim; delim != "" || len(delim) != 0 {
delimName = env.Name + ".delim"
}
return suffixName, delimName, err
}

func ParseBuildConfigEnv(env []builder.BuildConfigEnv, path string) (envMap map[string]string, warnings []string, err error) {
WYGIN marked this conversation as resolved.
Show resolved Hide resolved
envMap = map[string]string{}
var appendOrPrependWithoutDelim = 0
for _, v := range env {
if name := v.Name; name == "" || len(name) == 0 {
return nil, nil, errors.Wrapf(errors.Errorf("env name should not be empty"), "parse contents of '%s'", path)
}
if val := v.Value; val == "" || len(val) == 0 {
warnings = append(warnings, fmt.Sprintf("empty value for key/name %s", style.Symbol(v.Name)))
}
suffixName, delimName, err := getBuildConfigEnvFileName(v)
if err != nil {
return envMap, warnings, err
}
if val, e := envMap[suffixName]; e {
warnings = append(warnings, fmt.Sprintf(errors.Errorf("overriding env with name: %s and suffix: %s from %s to %s", style.Symbol(v.Name), style.Symbol(string(v.Suffix)), style.Symbol(val), style.Symbol(v.Value)).Error(), "parse contents of '%s'", path))
}
if val, e := envMap[delimName]; e {
warnings = append(warnings, fmt.Sprintf(errors.Errorf("overriding env with name: %s and delim: %s from %s to %s", style.Symbol(v.Name), style.Symbol(v.Delim), style.Symbol(val), style.Symbol(v.Value)).Error(), "parse contents of '%s'", path))
}
if delim := v.Delim; (delim != "" || len(delim) != 0) && (delimName != "" || len(delimName) != 0) {
envMap[delimName] = delim
}
envMap[suffixName] = v.Value
}

for k := range envMap {
name, suffix, err := getFilePrefixSuffix(k)
if err != nil {
continue
}
if _, ok := envMap[name+".delim"]; (suffix == "append" || suffix == "prepend") && !ok {
warnings = append(warnings, fmt.Sprintf(errors.Errorf("env with name/key %s with suffix %s must to have a %s value", style.Symbol(name), style.Symbol(suffix), style.Symbol("delim")).Error(), "parse contents of '%s'", path))
appendOrPrependWithoutDelim++
}
}
if appendOrPrependWithoutDelim > 0 {
return envMap, warnings, errors.Errorf("error parsing [[build.env]] in file '%s'", path)
}
return envMap, warnings, err
}
Loading
Loading