api/compat: switch to moby/moby#28057
Conversation
|
@Luap99 , could you please check system.go (https://github.com/containers/podman/pull/28057/changes#diff-2facc6ee4ed6c946594f6ecde156045ee8227e22a06e966273a199f79888c339) and tell me if that's how you expect the support for multiple versions to be done? |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
1 similar comment
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
Not a proper review but yeah looks about right logic wise |
|
Thanks, that's all I needed so far :-). |
d6a3860 to
c50fa7a
Compare
001b4f5 to
43ca938
Compare
dd2fdb5 to
b6d4436
Compare
502e398 to
1556b9f
Compare
|
If this is ready for review can you drop draft and wip title so others will take a look as well |
Luap99
left a comment
There was a problem hiding this comment.
LGTM overall though I have not checked stuff in detail
| StopTimeout: &stopTimeout, | ||
| Shell: nil, | ||
| }, | ||
| MacAddress: "", |
There was a problem hiding this comment.
this is not needed, by default it will always be the zero value (i.e. empty string)
but sure it does not hurt either
There was a problem hiding this comment.
I did that to stay consistent with the rest of the code where we explicitly set an empty string.
|
@Luap99 , I'm thinking if we could merge this one or if I should ask someone for additional review. What do you think? |
|
well we generally need two reviews unless it is something trivial so yeah... |
Honny1
left a comment
There was a problem hiding this comment.
I checked the code, and it seems good to me. (Just two questions.) I'm a little bothered by the structures that start with 'Legacy'. For now, it's okay, but for 6.0 we should throw them away or rename them differently or start deprecation, because the naming implies we're trying to support something old. Also, I have a feeling that we probably missed filling in some new fields in the structures, but tracking that seems pretty hard. I think the best effort approach is to wait until someone files an issue.
Maybe in the future, we should create a smoke test for the compat API to check if it works with Docker, ensuring it functions correctly for at least the basic use cases.
| ID: report.ID, | ||
| }) | ||
| if err == nil { | ||
| report.ProgressMessage = string(b) |
There was a problem hiding this comment.
The output from report.Progress.String() seems to be different from the marshaled JSON. Won't this break callers?
There was a problem hiding this comment.
That's good point. I double-checked the moby/moby code and you are right. I've backported the original String() function to our images_push.go. I think there's no other way around it, because this String() function is not part of public API anymore.
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@Honny1 , yeah. That compat code seems to be all over the place. Ideally we should just take the client versions we support and run the API calls with them to ensure they work with our API and report failures. Or really compare our responses with docker responses as part of tests. |
|
I've addressed the comments and rebased the PR. |
|
/packit retest-failed |
1 similar comment
|
/packit retest-failed |
|
@Honny1 do you think you could find some time to review this one again, please? :-) |
Honny1
left a comment
There was a problem hiding this comment.
I found docker/docker and other Docker libs in go.mod. Is this correct?
github.com/docker/distribution v2.8.3+incompatible
github.com/docker/docker v28.5.2+incompatible
github.com/docker/go-connections v0.6.0
github.com/docker/go-plugins-helpers v0.0.0-20240701071450-45e2431495c8
github.com/docker/go-units v0.5.0
There is still one package we depend on directly (some indirectly which would take even longer to get rid of), it should not block the API changes for now |
| VirtualSize int64 `json:"VirtualSize,omitempty"` | ||
| } | ||
|
|
||
| // Deprecated: kept to maintain backwards compatibility with API < v1.52 |
There was a problem hiding this comment.
The linter is flagging a deprecation notice. I would silence it when these deprecated structures are used.
Replace github.com/docker/docker API imports with github.com/moby/moby across compat handlers, swagger models, and tests to align with upstream type definitions. Fixes: containers#27536. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
The JSONProgress is not part of moby/moby API anymore: moby/moby@f4127d7 To stay compatible with the previous client version, this commit backports the jsonmessage.JSONProgress.String() and uses it to genereate the progress report. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
Replace github.com/docker/docker API imports with github.com/moby/moby across compat handlers, swagger models, and tests to align with upstream type definitions.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?