Don’t compute onbuild triggers for images that are stage names#938
Don’t compute onbuild triggers for images that are stage names#938dgageot merged 2 commits intoGoogleContainerTools:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #938 +/- ##
==========================================
+ Coverage 43.38% 43.55% +0.16%
==========================================
Files 63 63
Lines 2639 2645 +6
==========================================
+ Hits 1145 1152 +7
+ Misses 1374 1373 -1
Partials 120 120
Continue to review full report at Codecov.
|
balopat
left a comment
balopat
left a comment
There was a problem hiding this comment.
one testing related issue, otherwise LGTM
pkg/skaffold/docker/parse_test.go
Outdated
|
|
||
| func mockRetrieveImage(image string) (*v1.ConfigFile, error) { | ||
| if image == "base" { | ||
| panic("we should never retrieve images which are stage names") |
There was a problem hiding this comment.
panic is considered a bad practice in testing as it stops the test execution - can we just return an error, or pass in testing.T?
There was a problem hiding this comment.
also assertion logic should not be embedded in the mock, this makes a test hard to understand.
There was a problem hiding this comment.
suggestion: reorganize the mock with usual "capture" semantics, that records the calls as a slice of strings and make the retrieved images an explicit assertion in the tests.
Also verifies which images are actually fetched. Signed-off-by: David Gageot <david@gageot.net>
|
@balopat should be all good now. |
3 participants
Fix #502