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

Test & Fix build with rm/force-rm matrix #35139

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
6 participants
@simonferquel
Contributor

simonferquel commented Oct 9, 2017

fix #35116

- What I did

Added test cases for build related to rm/force-rm flags, and fixed a few related issues

- How I did it

Added a test with a case matrix in integration/build reproducing potential issues and fixed them in build code base

- How to verify it

run the tests :)

Note:
docker build rm and force-rm flags might be better documented:
It is unclear that on build failures, rm still remove successfull individual intermediate containers (only the failing one is kept).

@thaJeztah

LGTM, but I'd like @tonistiigi to have a look as well 👍

@tonistiigi

@andrewhsu We should take this for next v17.10 RC. lmk if you need cherry-pick prs.

cc @dnephin to check the test

Show outdated Hide outdated builder/dockerfile/builder.go Outdated
@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Oct 10, 2017

Contributor

@tonistiigi I centralized intermediate container removal logic in a single defer block in the dispatch() function with proper comments.
I think it makes it easier to understand the effect of rm/force-rm flags.
I'll make a PR in CLI that tries to make the -rm flag documentation more precise.

Contributor

simonferquel commented Oct 10, 2017

@tonistiigi I centralized intermediate container removal logic in a single defer block in the dispatch() function with proper comments.
I think it makes it easier to understand the effect of rm/force-rm flags.
I'll make a PR in CLI that tries to make the -rm flag documentation more precise.

Show outdated Hide outdated builder/dockerfile/evaluator.go Outdated
@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Oct 10, 2017

Contributor

@thaJeztah condition expression split :)

Contributor

simonferquel commented Oct 10, 2017

@thaJeztah condition expression split :)

@dnephin

thanks, fix looks good, left some suggestions for the tests

Show outdated Hide outdated integration/build/build_test.go Outdated
Show outdated Hide outdated integration/build/build_test.go Outdated
Show outdated Hide outdated integration/build/build_test.go Outdated
Show outdated Hide outdated integration/build/build_test.go Outdated
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Oct 10, 2017

Member

LGTM

Member

tonistiigi commented Oct 10, 2017

LGTM

@thaJeztah

one nit in the code; also can you check if some of the commits should be squashed?

Show outdated Hide outdated integration/build/build_test.go Outdated
@vdemeester

LGTM 🐸

@dnephin

minor nits on the test

LGTM

Show outdated Hide outdated integration/build/build_test.go Outdated
Show outdated Hide outdated integration/build/build_test.go Outdated
Show outdated Hide outdated integration/build/build_test.go Outdated
Test & Fix build with rm/force-rm matrix
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Oct 12, 2017

Contributor

@dnephin, @thaJeztah, latest nits are fixed, everything squashed and rebased

Contributor

simonferquel commented Oct 12, 2017

@dnephin, @thaJeztah, latest nits are fixed, everything squashed and rebased

@dnephin

LGTM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Oct 12, 2017

Member

powerpc failures mostly look like flakes, but the busybox missing flag is weird. I'll try restarting it.

Member

dnephin commented Oct 12, 2017

powerpc failures mostly look like flakes, but the busybox missing flag is weird. I'll try restarting it.

@vdemeester vdemeester merged commit 9a166a7 into moby:master Oct 13, 2017

5 of 6 checks passed

powerpc Jenkins build Docker-PRs-powerpc 6452 has encountered an error
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37374 has succeeded
Details
janky Jenkins build Docker-PRs 46063 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17638 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6251 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment