Updates to warnings handling #2498
Conversation
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
|
Some open questions/follow-ups:
|
Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
warnings-updates
branch
from
8cd777a to
71316c6
Compare
frontend/dockerfile/parser/parser.go
Outdated
| warnings = append(warnings, "[WARNING]: Empty continuation line found in:\n "+line) | ||
| warnings = append(warnings, Warning{ | ||
| Short: "Empty continuation line found in: " + line, | ||
| Detail: "Empty continuation lines will become errors in a future release. https://github.com/moby/moby/pull/33719", |
There was a problem hiding this comment.
@thaJeztah Is this actually marked as deprecated somewhere. Can't find it.
There was a problem hiding this comment.
Hmm.. good one; we should at least have it documented in https://docs.docker.com/engine/deprecated/ (looks like we don't), with more discussion in moby/moby#29005.
I think we decided to not remove it (at least for the time being), but to make the warning a bit more scary. Will have to dig those discussions though.
| @@ -138,7 +138,10 @@ message VertexLog { | |||
| message VertexWarning { | |||
| string vertex = 1 [(gogoproto.customtype) = "github.com/opencontainers/go-digest.Digest", (gogoproto.nullable) = false]; | |||
| int64 level = 2; | |||
There was a problem hiding this comment.
What's the purpose of level? Is it a threshold controlling how severe a warning must be?
There was a problem hiding this comment.
Yes, it is currently unused though but is propagated to the client if the frontend sets it.
Maybe |
|
@crazy-max At least initially I think all warnings will be from frontend, so defined in dockerfile in our case. I can add separate fields for URL (and code?) if that is desirable and we have ideas how to show them separately on cli. Currently, they are expected to just be inside "detail". I don't want to force some "automatic URLs based on code". If at all that should be on the client side, and other frontends may follow different logic. |
|
@tonistiigi Yes we can begin with frontend and let it decide how detail is defined. URL sgtm but I think it should be mutually exclusive with |
|
Updated with following changes:
|
Detail is now an array and URL is a separate field. Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
warnings-updates
branch
from
03a62fe to
872518e
Compare
| @@ -125,6 +125,11 @@ func (p *textMux) printVtx(t *trace, dgst digest.Digest) { | |||
| } | |||
| v.statusUpdates = map[string]struct{}{} | |||
|
|
|||
| for _, w := range v.warnings[v.warningIdx:] { | |||
| fmt.Fprintf(p.w, "#%d WARN: %s\n", v.index, w.Short) | |||
There was a problem hiding this comment.
Do you have an output example in progress ui (asciinema)? Also detail and url are not displayed in progress atm right? Are you thinking of displaying them only in debug mode?
There was a problem hiding this comment.
Yes, this just prints the (short) warning message under the vertex like a regular log from a process. The detail/url etc would need to be handled outside of the progress printing. The DisplaySolveStatus https://github.com/moby/buildkit/pull/2498/files#diff-c500feee78b97c9eba6731bee138b3d93ee24cf99f89733d1326b4f4d23e05cfR24 returns the list of all warnings after progress completes so whatever invoked it can do additional processing.
This PR extends #2482 with: