-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Refactor to remove dependencies on github.com/docker/docker/pkg/streamformatter and #13304
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
Refactor to remove dependencies on github.com/docker/docker/pkg/streamformatter and #13304
Conversation
This change copies the progress package from https://github.com/moby/moby/tree/f8215cc266744ef195a50a70d427c345da2acdbb/pkg/progress Signed-off-by: Austin Vazquez <[email protected]>
5d8c97e to
ee3e2bd
Compare
ee3e2bd to
b3a3372
Compare
…ress Signed-off-by: Austin Vazquez <[email protected]>
…matter This change copies the streamformatter package from https://github.com/moby/moby/tree/f8215cc266744ef195a50a70d427c345da2acdbb/pkg/streamformatter Signed-off-by: Austin Vazquez <[email protected]>
…amformatter Signed-off-by: Austin Vazquez <[email protected]>
…formatter Signed-off-by: Austin Vazquez <[email protected]>
Signed-off-by: Austin Vazquez <[email protected]>
b3a3372 to
496ef8d
Compare
glours
left a comment
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.
LGTM + tests ok
Why? |
ndeloof
left a comment
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.
don't copy code inside compose but use new api module
👍🏼 , @ndeloof, I added a writeup to the description to clarify. PTAL. |
|
is the docker/cli also supposed to use a local copy of this code then? |
|
I have opened docker/cli#6570 but no review yet from CLI folks. IIUC yes there probably can be a shared place between CLI and compose. Just not sure where that needs to be. |
|
I'm fine this code is relocated in docker/cli, I just wonder how it will be used if made internal in moby, as this is basically API-consumer code |
|
we already have docker/cli as a dependency, so if it eventually own (a copy of) this code, would be simpler we just us it from there vs have our own copy |
|
I've pushed a new revision to moby/moby#51153 to move the packages out of the api module up into the moby client. Probably long term we can investigate having these be in a better location perhaps CLI or some other Go SDK. For now this unblocks the API module release. Thanks! |
github.com/docker/docker/pkg/progress
What I did
This changes copies the streamformatter and progress packages from github.com/docker/docker/api. The upcoming Docker 29 release is introducing Go modules: moby/api, moby/client, and moby/moby. Previously part of the top level pkg package, the streamformatter and progress packages are being made internal daemon packages.
Related issue
Moby issue: moby/moby#50575
Moby PR: moby/moby#51153
(not mandatory) A picture of a cute animal, if possible in relation to what you did