Skip to content

bake: fix override bug with inheritance #259

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.

Sign up for GitHub

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

Merged
tonistiigi merged 2 commits into from
Apr 23, 2020
Merged

bake: fix override bug with inheritance #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3282dae
bake: add tests for override+inheritance bug
Apr 21, 2020
14e65ff
bake: fix override+inheritance bug
Apr 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions bake/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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] {
Expand All @@ -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{}
Expand All @@ -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{}
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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])
Copy link
Copy Markdown
Collaborator Author

@tiborvass tiborvass Apr 21, 2020

Choose a reason for hiding this comment

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

@tonistiigi I found another bug (not a regression from this PR). See test with VAR_BOTH. I believe this is correct: t is the child target, and tt is the parent, we want to merge the child into the parent, and the overrides on top.

Copy link
Copy Markdown
Collaborator Author

@tiborvass tiborvass Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

An important invariant I can add a comment in the code for, is that t should not have the default overrides (context = "." and dockerfile = "."), which is the case here because target() is called before setting those defaults in ResolveTarget.

tt.normalize()
return &tt, nil
}
Expand Down
69 changes: 51 additions & 18 deletions bake/bake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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))
Copy link
Copy Markdown
Collaborator Author

@tiborvass tiborvass Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

test hardening to catch a regression I had during dev


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",
Copy link
Copy Markdown
Collaborator Author

@tiborvass tiborvass Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this is a regression test for a regression I hit during writing the PR

"dep.args.VAR_BOTH=override",
Copy link
Copy Markdown
Collaborator Author

@tiborvass tiborvass Apr 21, 2020

Choose a reason for hiding this comment

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

this one is not, it's a bug in master.

})
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) {
Expand Down