-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Better support for docker build with a target #1497
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1497 +/- ##
==========================================
+ Coverage 45.18% 45.45% +0.27%
==========================================
Files 115 115
Lines 4794 4818 +24
==========================================
+ Hits 2166 2190 +24
Misses 2403 2403
Partials 225 225
Continue to review full report at Codecov.
|
bcf9d89
to
7765b0a
Compare
Fix GoogleContainerTools#1120 Signed-off-by: David Gageot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two nits but LGTM
return nodes, nil | ||
} | ||
|
||
byTarget := make(map[string][]*parser.Node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var byTarget map[string][]*parser.Node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Setting a value on a nil map fails hard
|
||
for _, node := range nodesByTarget[target] { | ||
if node.Value == command.From { | ||
inst := fromInstruction(node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could inline this into the append(...)
call instead of creating a new var
Fix #1120
Signed-off-by: David Gageot [email protected]