From 3282dae09b2bd194fa05a78126239c66742a8150 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 21 Apr 2020 19:01:18 +0000 Subject: [PATCH 1/2] bake: add tests for override+inheritance bug Signed-off-by: Tibor Vass --- bake/bake_test.go | 69 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/bake/bake_test.go b/bake/bake_test.go index c371a1db..cc004d0b 100644 --- a/bake/bake_test.go +++ b/bake/bake_test.go @@ -19,10 +19,17 @@ func TestReadTargets(t *testing.T) { fp := filepath.Join(tmpdir, "config.hcl") err = ioutil.WriteFile(fp, []byte(` target "dep" { + args { + VAR_INHERITED = "dep" + VAR_BOTH = "dep" + } } target "webapp" { dockerfile = "Dockerfile.webapp" + args { + VAR_BOTH = "webapp" + } inherits = ["dep"] }`), 0600) require.NoError(t, err) @@ -32,35 +39,61 @@ target "webapp" { t.Run("NoOverrides", func(t *testing.T) { m, err := ReadTargets(ctx, []string{fp}, []string{"webapp"}, nil) require.NoError(t, err) + require.Equal(t, 1, len(m)) require.Equal(t, "Dockerfile.webapp", *m["webapp"].Dockerfile) require.Equal(t, ".", *m["webapp"].Context) + require.Equal(t, "dep", m["webapp"].Args["VAR_INHERITED"]) + }) + + t.Run("InvalidTargetOverrides", func(t *testing.T) { + _, err := ReadTargets(ctx, []string{fp}, []string{"webapp"}, []string{"nosuchtarget.context=foo"}) + require.NotNil(t, err) + require.Equal(t, err.Error(), "unknown target nosuchtarget") }) t.Run("ArgsOverrides", func(t *testing.T) { - os.Setenv("VAR_FROMENV"+t.Name(), "fromEnv") - defer os.Unsetenv("VAR_FROM_ENV" + t.Name()) - - m, err := ReadTargets(ctx, []string{fp}, []string{"webapp"}, []string{ - "webapp.args.VAR_UNSET", - "webapp.args.VAR_EMPTY=", - "webapp.args.VAR_SET=bananas", - "webapp.args.VAR_FROMENV" + t.Name(), - }) - require.NoError(t, err) + t.Run("leaf", func(t *testing.T) { + os.Setenv("VAR_FROMENV"+t.Name(), "fromEnv") + defer os.Unsetenv("VAR_FROM_ENV" + t.Name()) - require.Equal(t, "Dockerfile.webapp", *m["webapp"].Dockerfile) - require.Equal(t, ".", *m["webapp"].Context) + m, err := ReadTargets(ctx, []string{fp}, []string{"webapp"}, []string{ + "webapp.args.VAR_UNSET", + "webapp.args.VAR_EMPTY=", + "webapp.args.VAR_SET=bananas", + "webapp.args.VAR_FROMENV" + t.Name(), + "webapp.args.VAR_INHERITED=override", + // not overriding VAR_BOTH on purpose + }) + require.NoError(t, err) + + require.Equal(t, "Dockerfile.webapp", *m["webapp"].Dockerfile) + require.Equal(t, ".", *m["webapp"].Context) + + _, isSet := m["webapp"].Args["VAR_UNSET"] + require.False(t, isSet, m["webapp"].Args["VAR_UNSET"]) - _, isSet := m["webapp"].Args["VAR_UNSET"] - require.False(t, isSet, m["webapp"].Args["VAR_UNSET"]) + _, isSet = m["webapp"].Args["VAR_EMPTY"] + require.True(t, isSet, m["webapp"].Args["VAR_EMPTY"]) - _, isSet = m["webapp"].Args["VAR_EMPTY"] - require.True(t, isSet, m["webapp"].Args["VAR_EMPTY"]) + require.Equal(t, m["webapp"].Args["VAR_SET"], "bananas") - require.Equal(t, m["webapp"].Args["VAR_SET"], "bananas") + require.Equal(t, m["webapp"].Args["VAR_FROMENV"+t.Name()], "fromEnv") - require.Equal(t, m["webapp"].Args["VAR_FROMENV"+t.Name()], "fromEnv") + require.Equal(t, m["webapp"].Args["VAR_BOTH"], "webapp") + require.Equal(t, m["webapp"].Args["VAR_INHERITED"], "override") + }) + + // building leaf but overriding parent fields + t.Run("parent", func(t *testing.T) { + m, err := ReadTargets(ctx, []string{fp}, []string{"webapp"}, []string{ + "dep.args.VAR_INHERITED=override", + "dep.args.VAR_BOTH=override", + }) + require.NoError(t, err) + require.Equal(t, m["webapp"].Args["VAR_INHERITED"], "override") + require.Equal(t, m["webapp"].Args["VAR_BOTH"], "webapp") + }) }) t.Run("ContextOverride", func(t *testing.T) { From 14e65ff3b46161d1e1533af7e3c12175be83f277 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 21 Apr 2020 19:53:19 +0000 Subject: [PATCH 2/2] bake: fix override+inheritance bug Signed-off-by: Tibor Vass --- bake/bake.go | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/bake/bake.go b/bake/bake.go index 945288ff..6ec277f9 100644 --- a/bake/bake.go +++ b/bake/bake.go @@ -23,13 +23,14 @@ func ReadTargets(ctx context.Context, files, targets, overrides []string) (map[s } c = mergeConfig(c, *cfg) } - if err := c.setOverrides(overrides); err != nil { + o, err := c.newOverrides(overrides) + if err != nil { return nil, err } m := map[string]Target{} for _, n := range targets { for _, n := range c.ResolveGroup(n) { - t, err := c.ResolveTarget(n) + t, err := c.ResolveTarget(n, o) if err != nil { return nil, err } @@ -105,23 +106,24 @@ func mergeConfig(c1, c2 Config) Config { return c1 } -func (c Config) setOverrides(v []string) error { +func (c Config) newOverrides(v []string) (map[string]Target, error) { + m := map[string]Target{} for _, v := range v { parts := strings.SplitN(v, "=", 2) keys := strings.SplitN(parts[0], ".", 3) if len(keys) < 2 { - return errors.Errorf("invalid override key %s, expected target.name", parts[0]) + return nil, errors.Errorf("invalid override key %s, expected target.name", parts[0]) } name := keys[0] if len(parts) != 2 && keys[1] != "args" { - return errors.Errorf("invalid override %s, expected target.name=value", v) + return nil, errors.Errorf("invalid override %s, expected target.name=value", v) } - t, ok := c.Target[name] - if !ok { - return errors.Errorf("unknown target %s", name) + t := m[name] + if _, ok := c.Target[name]; !ok { + return nil, errors.Errorf("unknown target %s", name) } switch keys[1] { @@ -131,7 +133,7 @@ func (c Config) setOverrides(v []string) error { t.Dockerfile = &parts[1] case "args": if len(keys) != 3 { - return errors.Errorf("invalid key %s, args requires name", parts[0]) + return nil, errors.Errorf("invalid key %s, args requires name", parts[0]) } if t.Args == nil { t.Args = map[string]string{} @@ -146,7 +148,7 @@ func (c Config) setOverrides(v []string) error { } case "labels": if len(keys) != 3 { - return errors.Errorf("invalid key %s, lanels requires name", parts[0]) + return nil, errors.Errorf("invalid key %s, lanels requires name", parts[0]) } if t.Labels == nil { t.Labels = map[string]string{} @@ -170,11 +172,11 @@ func (c Config) setOverrides(v []string) error { case "output": t.Outputs = append(t.Outputs, parts[1]) default: - return errors.Errorf("unknown key: %s", keys[1]) + return nil, errors.Errorf("unknown key: %s", keys[1]) } - c.Target[name] = t + m[name] = t } - return nil + return m, nil } func (c Config) ResolveGroup(name string) []string { @@ -197,8 +199,8 @@ func (c Config) group(name string, visited map[string]struct{}) []string { return targets } -func (c Config) ResolveTarget(name string) (*Target, error) { - t, err := c.target(name, map[string]struct{}{}) +func (c Config) ResolveTarget(name string, overrides map[string]Target) (*Target, error) { + t, err := c.target(name, map[string]struct{}{}, overrides) if err != nil { return nil, err } @@ -213,7 +215,7 @@ func (c Config) ResolveTarget(name string) (*Target, error) { return t, nil } -func (c Config) target(name string, visited map[string]struct{}) (*Target, error) { +func (c Config) target(name string, visited map[string]struct{}, overrides map[string]Target) (*Target, error) { if _, ok := visited[name]; ok { return nil, nil } @@ -224,7 +226,7 @@ func (c Config) target(name string, visited map[string]struct{}) (*Target, error } var tt Target for _, name := range t.Inherits { - t, err := c.target(name, visited) + t, err := c.target(name, visited, overrides) if err != nil { return nil, err } @@ -233,7 +235,7 @@ func (c Config) target(name string, visited map[string]struct{}) (*Target, error } } t.Inherits = nil - tt = merge(merge(defaultTarget(), t), tt) + tt = merge(merge(merge(defaultTarget(), tt), t), overrides[name]) tt.normalize() return &tt, nil }