-
Notifications
You must be signed in to change notification settings - Fork 481
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
build: display Dockerfile path on check warnings #2672
build: display Dockerfile path on check warnings #2672
Conversation
6111e72
to
b152f27
Compare
0a4a8da
to
8967c5d
Compare
fccf749
to
474b651
Compare
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 think a problem with this one is that we are trying to parse the inputs from the user twice and assume that they get the same result. One is happening internally when builder converts input to LocalMounts
inside loadInputs
(that can create temp directories etc.), and other now here on converting the path for printing.
A better solution could be that when loadInputs
sets up local mount by calling setLocalMount
, then that mapping is then saved in that function so that it can be reversed later when printing. I think sourceInfo also has the Definition
block so we can actually confirm the name of the local and match it with correct client directory.
This is better because:
- Recude duplication and possible fragmentation where one side gets changed or some formats are not supported in one implementation.
- Cases like inline Dockerfiles and stdin can show "(inline)" as a filename instead of bogus "Dockerfile".
- It also works with named contexts, instead of correct hardcoded context+dockerfile variants.
If this is considered too complex for initial PR we could leave that solution to future follow-up, but in that case I think we should only have exception for the one path, where we already define the name for the Dockerfile we expect to receive in the commands
pkg and only pass this name instead of both raw contextPath
and dockerfileName
and then trying to figure out what is remote and how to combine them while printing. We should at least handle -
as well.
Not too complex, I think! Ty you for the review! |
474b651
to
b212eed
Compare
23afc53
to
73c0fb1
Compare
…g rule check warnings Signed-off-by: Talon Bowler <[email protected]>
62d6261
to
f1b92e9
Compare
Massively refactored in order to take the approach suggested by @tonistiigi. :) |
build/opt.go
Outdated
} | ||
|
||
func loadInputs(ctx context.Context, d *driver.DriverHandle, inp Inputs, pw progress.Writer, target *client.SolveOpt) (func(), error) { | ||
type DockerfileMapping struct { |
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.
Instead of returning map of these mappings, can't it be part of the Inputs
object and loadInputs
would save it there. Then it can be accessed from the build.Options
passed from the commands
.
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.
Updated to do it this way - I still ended up returning a build.Inputs
because the top-level calls we need to return this information from use controllerapi.BuildOptions
as an arg, and I didn't think updating that was the right call (because adding the info to GRPC-land seems like the wrong approach when we don't need to communicate this information across client/server boundary).
build/opt.go
Outdated
@@ -540,7 +551,7 @@ func loadInputs(ctx context.Context, d *driver.DriverHandle, inp Inputs, pw prog | |||
_ = os.RemoveAll(dir) | |||
} | |||
} | |||
return release, nil | |||
return release, &DockerfileMapping{Src: dockerfileSrcName, Dst: dockerfileName}, nil |
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 thought these mappings would also be for context and namedcontext but I guess for this it is not needed. In some other cases with source mappings in error it could be maybe possible, but not needed for this PR.
ac85b1e
to
1b85289
Compare
dockerfileSrcName = "inline" | ||
} else if inp.DockerfilePath == "-" { | ||
dockerfileSrcName = "stdin" |
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.
What will be the output for inline
and stdin
case? I guess it should be omitted like:
Command 'copy' should match the case of the command majority (uppercase)
--------------------
1 | FROM alpine
2 | >>> copy ./tmp /tmp
3 |
4 |
--------------------
Something like this would be confusing:
Command 'copy' should match the case of the command majority (uppercase)
inline:2
--------------------
1 | FROM alpine
2 | >>> copy ./tmp /tmp
3 |
4 |
--------------------
If someone has a Dockerfile named inline
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.
Removing the line entirely requires either a buildkit
PR or refactoring how buildx
is printing warnings to effectively duplicate that functionality. I'm not opposed, but if this is a strong ask then it might be best to merge this and then make a corresponding issue in buildkit
for the follow-up work.
I was trying to model what @tonistiigi had suggested, here. For MY part, I think that ambiguity around two very specifically named Dockerfiles (in this case, stdin
and inline
) is less confusing than simply not showing anything in these cases. That being said, it could be updated to something a little more clear and even less likely to be a filename?
Command 'copy' should match the case of the command majority (uppercase)
Inline Dockerfile:2
--------------------
...
Command 'copy' should match the case of the command majority (uppercase)
Dockerfile (inline):2
--------------------
...
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.
That being said, it could be updated to something a little more clear and even less likely to be a filename?
Yes that sounds good, let's check that for a follow-up then
@crazy-max noted that this does not work for the |
@tonistiigi Looking for a review on this bad boy still. :) |
1b85289
to
84ac4b9
Compare
@daghack PTAL linter error |
…puts through Build Signed-off-by: Talon Bowler <[email protected]>
84ac4b9
to
671bd1b
Compare
Ah, RIP. I now have an opinion about |
fixes #2542
Currently, when printing warnings from the rule checks, it will only report the base Dockerfile name.
docker build --check -f foo/bar/Dockerfile .
This updates printing for the warnings to include a more specific path if one is available.