compat: Implement deprecations from Docker API v1.44#28787
Conversation
|
It's looks like that the mocked output with EDIT: |
e6c98e5 to
e3be1af
Compare
inknos
left a comment
There was a problem hiding this comment.
Most likely you also want some versioned tests against the compat endpoints like in this previous PR about deprecating endpoints. https://github.com/containers/podman/pull/26213/changes
also, minor thing, the fixup commit could be squashed imho
| // https://docs.docker.com/reference/api/engine/version-history/#v144-api-changes | ||
| if _, err := apiutil.SupportedVersion(r, ">=1.44.0"); err == nil && !utils.IsLibpodRequest(r) { | ||
| delete(query.Filters, "is-automated") | ||
| } |
There was a problem hiding this comment.
stated in docker's api
The deprecation is not versioned, and applies to all API versions.
therefore I believe it should just be deleted, not only if version >= 1.44
| } | ||
|
|
There was a problem hiding this comment.
probably the Automated field should be zeroed out before writing to the response since it's documented to be always false. also, I would verify what docker wants, probably a bool and not a string. Then I would add a specific test for this field and its type
There was a problem hiding this comment.
@inknos I thought the agreement was not to zero it and be transparent - just return what's received in the response. The docs state:
will always be set to false in the future
and I think that if Docker Hub returns true for some reason (IIRC it happened when testing), it should not be changed to false here. Now, the Docker structure is used directly for the response.
But I can zero it if you think that's better.
| Stars int | ||
| // Official indicates if it's an official image. | ||
| Official string | ||
| // Deprecated: the "is_automated" field is deprecated and will always be "false". |
There was a problem hiding this comment.
we are not really deprecating this, but we want to touch the compat struct only, which we don't have. Therefore, I believe it's better to create a compat struct with the correct docker types and the deprecation should happen there. this struc is used elsewhere and we don't deprecate this option for the libpod code
|
Thank you, @inknos Regarding
[
{
"Index": "docker.io",
"Name": "docker.io/library/alpine",
"Description": "A minimal Docker image based on Alpine Linux with a complete package index and only 5 MB in size!",
"Stars": 11509,
"Official": "[OK]",
"Automated": "",
"Tag": ""
},
{
"Index": "docker.io",
"Name": "docker.io/alpine/git",
"Description": "A simple git container running in alpine linux, especially for tiny linux distro.",
"Stars": 252,
"Official": "",
"Automated": "[OK]",
"Tag": ""
},
{
"Index": "docker.io",
"Name": "docker.io/alpine/socat",
"Description": "Run socat command in alpine container",
"Stars": 115,
"Official": "",
"Automated": "[OK]",
"Tag": ""
},
{
"Index": "docker.io",
"Name": "docker.io/alpine/curl",
"Description": "",
"Stars": 12,
"Official": "",
"Automated": "",
"Tag": ""
},
{
"Index": "docker.io",
"Name": "docker.io/alpine/helm",
"Description": "Auto-trigger docker build for kubernetes helm when new release is announced",
"Stars": 70,
"Official": "",
"Automated": "",
"Tag": ""
}
]and the Docker one like this: [
{
"star_count": 11509,
"is_official": true,
"name": "alpine",
"is_automated": false,
"description": "A minimal Docker image based on Alpine Linux with a complete package index and only 5 MB in size!"
},
{
"star_count": 252,
"is_official": false,
"name": "alpine/git",
"is_automated": false,
"description": "A simple git container running in alpine linux, especially for tiny linux distro."
},
{
"star_count": 115,
"is_official": false,
"name": "alpine/socat",
"is_automated": false,
"description": "Run socat command in alpine container"
},
{
"star_count": 12,
"is_official": false,
"name": "alpine/curl",
"is_automated": false,
"description": ""
},
{
"star_count": 70,
"is_official": false,
"name": "alpine/helm",
"is_automated": false,
"description": "Auto-trigger docker build for kubernetes helm when new release is announced"
}
]
What do you think? (The EDIT:
|
|
restarted the failing tests. |
e3be1af to
41f83b4
Compare
[
{
"star_count": 11513,
"is_official": true,
"name": "alpine",
"is_automated": false,
"description": "A minimal Docker image based on Alpine Linux with a complete package index and only 5 MB in size!"
}
]
[
{
"star_count": 11513,
"is_official": true,
"name": "docker.io/library/alpine",
"is_automated": false,
"description": "A minimal Docker image based on Alpine Linux with a complete package index and only 5 MB in size!"
}
]
[
{
"Index": "docker.io",
"Name": "docker.io/library/alpine",
"Description": "A minimal Docker image based on Alpine Linux with a complete package index and only 5 MB in size!",
"Stars": 11513,
"Official": "[OK]",
"Automated": "",
"Tag": ""
}
] |
41f83b4 to
e8c9311
Compare
| i = 1 | ||
| # Need to explicitly set start method | ||
| # # https://docs.python.org/dev/library/multiprocessing.html#contexts-and-start-methods | ||
| mp.set_start_method('fork') |
There was a problem hiding this comment.
The multiprocessing sub-tests weren't actually correctly asserted and they failed silently. Simplified that with sequential sub-tests.
There was a problem hiding this comment.
@simek-m you should rebase to main if you didn't so ci jobs will show up and run the tests
There was a problem hiding this comment.
@inknos I did, but I think the CI workflow needs to be approved.
There was a problem hiding this comment.
don't see the ci runs, but if you do, everything's correct, so apologies if that's the case and if you did rebase already 🥲
…dpoint The Docker API in version 1.44 deprecates the is_automated field for the GET /images/search endpoint. The is_automated field has been deprecated by Docker Hub's search API. Consequently, searching for is-automated=true will yield no results. The Docker API in version 1.44 deprecates the is_automated field in the GET /images/search response and will always be set to false in the future because Docker Hub is deprecating the is_automated field in its search API. Return struct moby/api for the compat endpoint that matches the Docker API response format and deprecates is_automated. Update test_v2_0_0_image.py::ImageTestCase::test_search_compat to verify returned format and fix subtests not being asserted (remove mp). Fixes: https://redhat.atlassian.net/browse/RUN-3323 Signed-off-by: Marek Simek <msimek@redhat.com>
e8c9311 to
c3ed8ec
Compare
|
I'd wait for the ci to be run @Honny1 do you have permissions to approve? then LGTM |
CI is green. |
|
PTAL @podman-container-tools/podman-maintainers |
It's trickier with the
But version
TLDR.: I remove the deprecations from shared structures and try to introduce a version gating for |
we have to go there eventually, but since we are ~8 versions away from 1.52 I hope this does not introduce more confusion going forward to what's fixed and what's not. I would say to keep it simple, test what we can concerning 1.44 and move on |
The Docker API 1.44 deprecates the fields HairpinMode, LinkLocalIPv6Address,
LinkLocalIPv6PrefixLen, SecondaryIPAddresses, SecondaryIPv6Addresses available in
NetworkSettings when calling GET /containers/{id}/json and will be removed in a future release.
You should instead look for the default network in NetworkSettings.Networks.
The fields are removed in 1.52. Version gate SecondaryIPAddresses, SecondaryIPv6Addresses
in the handler and update test. HairpinMode, LinkLocalIPv6Address, LinkLocalIPv6PrefixLen
are not returned by the compat endpoint as the response is serialized
to the moby/moby/api structure missing these fields.
Fixes: https://redhat.atlassian.net/browse/RUN-3323
Signed-off-by: Marek Simek <msimek@redhat.com>
…{name}/json
The Docker API deprecates Container and ContainerConfig fields in
the GET /images/{name}/json response are deprecated and
they will no longer be included in API v1.45.
Fixes: https://redhat.atlassian.net/browse/RUN-3323
Signed-off-by: Marek Simek <msimek@redhat.com>
c3ed8ec to
f9dccd1
Compare
@inknos @Honny1
|
Honny1
left a comment
There was a problem hiding this comment.
Just a possible linter issue.
|
Updated the release notes - I forgot to make it clear that it relates to the Compat endpoints. |

This PR implements API deprecations to be compatible with Docker API v1.44.
ContainerandContainerConfigfrom GET/images/{name}/jsonHairpinMode,LinkLocalIPv6Address,LinkLocalIPv6PrefixLen,SecondaryIPAddresses,SecondaryIPv6Addressesfrom GET/containers/{id}/jsonis-automated filterfor the GET/images/searchendpointDeprecations in Docker Engine API v1.44 API
Fixes: https://redhat.atlassian.net/browse/RUN-3323
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?
Questions and Notes
is_automatedare according to the Docker docsnot versioned, and apply to all API versions. Hence, I'm not sure if my approach is correct.ContainerConfigfield forv1.45so I kept the version. [compat api] Remove ContainerConfig field #27172is_automatedand I kept it there.