builder: Make builtin arg pruning work with > 1 arg#32763
builder: Make builtin arg pruning work with > 1 arg#32763cpuguy83 merged 1 commit intomoby:masterfrom
Conversation
|
/cc @thaJeztah - this will need cherry picking in to the next RC for 17.05 |
builder/dockerfile/dispatchers.go
Outdated
There was a problem hiding this comment.
Oh we're appending to a new slice instead of removing from the slice
There was a problem hiding this comment.
An integration test that has to compile the entire daemon, start a daemon and run a full build is somewhat overkill for testing an array out of bounds error.
Could this be a unit test for run() ?
There was a problem hiding this comment.
You could argue that it's not just testing the oob error but that it's testing the use case...
If you are against having a dedicated test for it, I can collapse it in to the previous build-arg exclusions test.
As for adding a unit test, I'll take a look and see how easy that'll be...
There was a problem hiding this comment.
It is definitely a test for that use case, but we can't write a slow integration test for every possible input. The test suite would take multiple years to run.
Unfortunately I don't think a unit test is going to be possible right now. I wrote one in #32773 after I refactored a lot of this code. Currently this code relies on runConfig being shared with the container config of a conatiner that was already created. It's effectively modifying the config of a container post-create, which to me is a huge violation of the boundaries between components.
After #32773 the updated cmd is passed as an argument to commit.
For now updating the existing test sounds good.
The previous implementation would error out with "Unexpected EOF" which was caused by an underlying "array index out-of-bounds" error. The root cause was deleting items from the same array that was being iterated over. The iteration was unaware that the array size had changed, resulting in an error. The new implementation builds a new array instead of mutating a copy of the old one. Fixes: moby#32744 Signed-off-by: Dave Tucker <[email protected]>
vdemeester
added
process/cherry-pick
status/2-code-review
and removed
status/0-triage
labels
|
LGTM - I have a few comments (for readability) that I'll add later, but nothing blocking. |
|
has the same problem |
10 participants
The previous implementation would error out with "Unexpected EOF" which
was caused by an underlying "array index out-of-bounds" error.
The root cause was deleting items from the same array that was being
iterated over. The iteration was unaware that the array size had
changed, resulting in an error.
The new implementation builds a new array instead of mutating a copy of
the old one.
Fixes: #32744
Signed-off-by: Dave Tucker [email protected]