Move jsonmessage, streamformatter, and progress #50565
Conversation
02bd816 to
faa2a31
Compare
|
|
||
| // an integer error code, Message is the error message. | ||
| type JSONError struct { | ||
| Code int `json:"code,omitempty"` |
There was a problem hiding this comment.
(for follow-ups); not sure if we currently do, but we should have some code in handling the stream that converts this back to an errdef; perhaps with errdefs.ToNative (although that doesn't allow preserving the error-message).
https://github.com/containerd/errdefs/blob/4817405e4a3caeb7aee9dac68ed55339c59cb635/pkg/errhttp/http.go#L69)
pkg/jsonmessage/jsonmessage.go
Outdated
| Time int64 `json:"time,omitempty"` | ||
| TimeNano int64 `json:"timeNano,omitempty"` | ||
| Error *JSONError `json:"errorDetail,omitempty"` | ||
| ProgressMessage string `json:"progress,omitempty"` |
There was a problem hiding this comment.
I'll have a look in a follow-up to remove the ProgressMessage and ErrorMessage field from the pkg/jsonmessage package. For the API server it looks like we need to keep those fields (they're LONG deprecated, but were always kept in the response, and docker-py did still depend on it at least in a test (but maybe ONLY in a test).
For the client, nothing should depend on it, so we can remove it as the same information is available in the replacement fields.
| type Progress struct { | ||
| Current int64 `json:"current,omitempty"` // Current is the current status and value of the progress made towards Total. | ||
| Total int64 `json:"total,omitempty"` // Total is the end value describing when we made 100% progress for an operation. | ||
| Start int64 `json:"start,omitempty"` // Start is the initial value for the operation. | ||
| HideCounts bool `json:"hidecounts,omitempty"` // HideCounts. if true, hides the progress count indicator (xB/yB). | ||
| Units string `json:"units,omitempty"` // Units is the unit to print for progress. It defaults to "bytes" if empty. | ||
| } |
There was a problem hiding this comment.
After this, we need to do some digging; look like the API specification only documents current and total and none of the others. HideCounts is also a bit of an odd one if it's indeed in the API response (feels more like a client decision, but I guess it is what it is);
Lines 2841 to 2847 in 2574c2b
| const RFC3339NanoFixed = "2006-01-02T15:04:05.000000000Z07:00" | ||
|
|
||
| // JSONProgress describes a progress message in a JSON stream. | ||
| type JSONProgress struct { |
There was a problem hiding this comment.
In a follow-up, we may even be able to un-export this struct (more so because it has unexpected fields)
There was a problem hiding this comment.
I'm still a bit confused how "jsonstream.Progress" and "progress.Progress" relate to each-other.
Considering if we should move the progress.Progress type to types/xxx - if that's the struct that's sent over the wire, it should correspond with something in the swagger file, so in that case it would make sense to have it somewhere under types/
There was a problem hiding this comment.
I have a feeling that only half of this file is "API" (there's the NewJSONProgressOutput which writes the JSON stream to an out-stream), the other half (NewProgressOutput, using the rawProgressFormatter) looks unused in this repository, but used in the CLI to format the stream (and could possibly replace some occurrences of the jsonstream package (? not sure).
There was a problem hiding this comment.
Used by compose as well. Considering the API allows the client to not support the json output, it should be considered part of the API. It doesn't need to be touched though and the output should not change
There was a problem hiding this comment.
Looks indeed that compose uses all three packages here for the classic builder;
| func rawProgressString(p *jsonstream.Progress) string { | ||
| if p == nil || (p.Current <= 0 && p.Total <= 0) { |
There was a problem hiding this comment.
Also see my other comment - it looks like this code may be better for the client/ (and after some more work, perhaps would even be able to replace uses of jsonmessage if they indeed do the same.
2056952 to
83b1c8e
Compare
|
Green, we can get this one in. For further splitting and moving, it shouldn't be necessary as this code should mostly be static and stable after this move. |
Also rename api type JSONError to Error Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Derek McGowan <derek@mcg.dev>
Move the type to the API, but embed it, so that we keep the methods on the struct in this package. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This package depends on jsonformatter.JSONProgress and jsonmessage.JSONMessage, and it looks like it requires some of those for their stringer interface. Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
334c134 to
0d8ca8e
Compare
|
FWIW; I created a ticket with remaining questions / things to look at; have a peek to see if any of those are possible before this PR, or to be done after; (latest push to this PR branch was to cleanup some commits, and a rebase after some other changes went in) |
|
LGTM but won't let me approve my own PR, please just get this merged |
|
OK let's bring this in, and move from here. Follow ups likely will be needed, but at least we could start trying the modules in the intermediate state |
Moves streamformatter and progress into api/pkg along with stdcopy. These packages are needed form and consume API responses.
Moves jsonmessage into client package, this is not needed by daemon or no longer needed by streamformatter. Clients use this package to display output based on the streamformatter.
Continuation of #50564