Deprecated and moved writeflusher impl to internal pkg #47718
Conversation
0ad6eb4 to
e76e088
Compare
e76e088 to
b4ebe28
Compare
…Flusher Signed-off-by: Christopher Petito <47751006+krissetto@users.noreply.github.com>
…riteHeader() on first Flush()) Signed-off-by: Christopher Petito <47751006+krissetto@users.noreply.github.com>
b4ebe28 to
d020e95
Compare
| // NewLegacyWriteFlusher returns a new LegacyWriteFlusher. | ||
| // | ||
| // Deprecated: use the internal writeflusher.NewWriteFlusher() instead | ||
| func NewLegacyWriteFlusher(w io.Writer) *LegacyWriteFlusher { |
There was a problem hiding this comment.
This is still used in a bunch of places in API, which makes the linter complain:
api/server/router/build/build_routes.go:234:12: SA1019: ioutils.NewWriteFlusher is deprecated: use the internal/writeflusher WriteFlusher instead. (staticcheck)
output := ioutils.NewWriteFlusher(ww)
...
we should also replace these with the new implementation.
|
|
||
| // NewLegacyWriteFlusher returns a new LegacyWriteFlusher. | ||
| // | ||
| // Deprecated: use the internal writeflusher.NewWriteFlusher() instead |
There was a problem hiding this comment.
Do we need to add the deprecation and removal notice to https://github.com/docker/cli/blob/master/docs/deprecated.md? @thaJeztah
While not strictly an API, if we're bothering with the deprecation-cycle anyway, perhaps it wouldn't hurt to call it out in our docs.
There was a problem hiding this comment.
This PR was created as a solution to the "superfluous call to writeheader" issue. It seemed like a good moment to go ahead and deprecate the old writeflusher implementation which was exposing some known buggy functionality (
moby/pkg/ioutils/writeflusher.go
Lines 54 to 64 in ae976b9
If we merge #47796, this PR will no longer be necessary unless we still want to deprecate the old writeflusher implementation
What I did
Flush()func to also callWriteHeader()on the underlying io.Writer if it's anhttp.ResponseWriter.This is needed to ensure that we don't try to send headers multiple times if the writer has already been wrapped by OTEL instrumentation (which doesn't wrap the
Flush()func. See Syslog entry: "superfluous response.WriteHeader call" in Docker 25.x #47448)How I did it
I moved the old impl to
internal/writeflusher, and created aNewLegacyWriteFlusher()func that the old exportedNewWriteFlusher()func calls to maintain the original behavior. All code using this deprecated version from theioutilspkg should be unaffectedHow to verify it
Usual tests should run as usual, as this shouldn't have any effect
Description for the changelog
A picture of a cute animal (not mandatory but encouraged)
