Skip to content

bake: fix override bug with inheritance#259

Merged
tonistiigi merged 2 commits intodocker:masterfrom
tiborvass:fix-inherits-override
Apr 23, 2020
Merged

bake: fix override bug with inheritance#259
tonistiigi merged 2 commits intodocker:masterfrom
tiborvass:fix-inherits-override

Conversation

@tiborvass
Copy link
Copy Markdown
Collaborator

@tiborvass tiborvass commented Apr 21, 2020 •
edited
Loading

Added the bug case in tests:

target "dep" {
	args {
		VAR_INHERITED = "dep"
	}
}
target "webapp" {
	dockerfile = "Dockerfile.webapp"
	inherits = ["dep"]
}

bake --set webapp.args.VAR_INHERITED=override was still inheriting dep value.

@tiborvass tiborvass requested a review from tonistiigi April 21, 2020 19:56
@tiborvass tiborvass added this to the v0.4.0 milestone Apr 21, 2020
tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake.go Outdated
@tiborvass tiborvass force-pushed the fix-inherits-override branch from fe74b65 to 28986b7 Compare April 21, 2020 20:06
tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake_test.go
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

@tiborvass tiborvass marked this pull request as draft April 21, 2020 20:28
@tiborvass
Copy link
Copy Markdown
Collaborator Author

tiborvass commented Apr 21, 2020 •
edited
Loading

Found another bug:

bake --set webfoo.args.VAR_INHERITED=override (parent overriding) does not override when building webapp.

EDIT: was regression in a previous version of my PR, added a test case.

@tiborvass tiborvass force-pushed the fix-inherits-override branch from 28986b7 to a1b6cff Compare April 21, 2020 21:14
@tiborvass tiborvass marked this pull request as ready for review April 21, 2020 21:14
tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake_test.go
// 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

@tiborvass tiborvass force-pushed the fix-inherits-override branch from a1b6cff to 78f869d Compare April 21, 2020 21:25
tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake.go Outdated
}
t.Inherits = nil
tt = merge(merge(defaultTarget(), t), tt)
tt = merge(merge(merge(defaultTarget(), t), tt), 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.

Note: nothing is setting c.Target[name] (aka t here), so I believe we could remove the defaultTarget() merge and do merge(t, tt) directly.

@tiborvass tiborvass force-pushed the fix-inherits-override branch from 78f869d to c623b89 Compare April 21, 2020 21:59
@tiborvass tiborvass marked this pull request as draft April 21, 2020 22:27
@tiborvass tiborvass force-pushed the fix-inherits-override branch from c623b89 to fc713b4 Compare April 21, 2020 22:35
@tiborvass tiborvass marked this pull request as ready for review April 21, 2020 22:36
bake: add tests for override+inheritance bug
3282dae 
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the fix-inherits-override branch from fc713b4 to 4388f14 Compare April 21, 2020 22:38
bake: fix override+inheritance bug
14e65ff 
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the fix-inherits-override branch from 4388f14 to 14e65ff Compare April 21, 2020 22:42
tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake.go
}
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.

tiborvass
tiborvass commented Apr 21, 2020
View reviewed changes
Comment thread bake/bake_test.go
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",
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.

@tiborvass tiborvass mentioned this pull request Apr 21, 2020
Merged
@tonistiigi tonistiigi merged commit 721b63f into docker:master Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@tiborvass @tonistiigi