api/search: Reset is_automated to false#47465
Conversation
vvoland
added
area/api
| // "is-automated" is deprecated and was removed in 1.45. | ||
| // If the client is older than 1.45, remove the filter to avoid erroring out | ||
| // on older clients. | ||
| if versions.LessThan(httputils.VersionFromContext(ctx), "1.45") { | ||
| for _, v := range searchFilters.Get("is-automated") { | ||
| searchFilters.Del("is-automated", v) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Following our discussion on Slack; I think the plan was to keep the filter, but it would not produce results; from the original PR;
Consequently, searching for
is-automated=truewill yield no results.
And the change to have IsAutomated always being false would be unversioned;
The deprecation is not versioned, and applies to all API versions.
Motivation there was that the deprecation correlates with the upstream (docker hub) service being in process of deprecating it; IIRC, the field is already out of date with reality, so results are not correct in many cases, thus always setting it to false makes sure that we don't return "bogus" results here.
There was a problem hiding this comment.
Yeah so with this PR the IsAutomated response will always be false (regardless of API version), but the is-automated filter will always produce an error on API >= 1.45 (it no longer makes sense, so we don't want newer clients to be using it while still keeping old clients working).
vvoland
force-pushed
the
v26-remove-deprecated
branch
2 times, most recently
from
b7c982f to
8c1eb86
Compare
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
If we're going to make is-automated=false an invalid filter on v1.45+ we should be upfront about that.
| // "is-automated" is deprecated and will always be false | ||
| result.IsAutomated = false //nolint:staticcheck // ignore SA1019 (field is deprecated) |
There was a problem hiding this comment.
If it's deprecated and always false in Hub results, why do we need to explicitly set it to false ourselves?
There was a problem hiding this comment.
IIUC, on Hub side this is not always false (yet), it's just deprecated.
But @thaJeztah might have more context here
|
I don't think we need to remove the filter, and just silently ignoring would be fine. The reason I'd want the field to be reset is that it's very much broken. The v1 search is a bit of a legacy service, and this field is known to be broken. In all honesty, it was announced internally to deprecated that field (as repairing would not be possible), but that task has been kicked around to different teams, so I have no clue when that work continues, other than knowing it's broken, and that it won't be fixed. |
What's the point of having that filter on |
vvoland
force-pushed
the
v26-remove-deprecated
branch
from
8c1eb86 to
b1039b5
Compare
|
|
And it won't break if they pin to an API version before 1.45, where it will just be silently ignored: moby/api/server/router/image/image_routes.go Lines 492 to 499 in b1039b5 |
vvoland
force-pushed
the
v26-remove-deprecated
branch
from
b1039b5 to
0e814c8
Compare
vvoland
changed the title
is-automated filteris_automated to false
|
As discussed in the maintainers call, updated the PR to not remove the filter completely. |
vvoland
force-pushed
the
v26-remove-deprecated
branch
2 times, most recently
from
007ae32 to
0f97a09
Compare
The field will still be present in the response, but will always be `false`. Searching for `is-automated=true` will yield no results, while `is-automated=false` will effectively be a no-op. Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
v26-remove-deprecated
branch
from
0f97a09 to
b292150
Compare
3 participants
The field will still be present in the response, but will always be
false.Searching for
is-automated=truewill yield no results, whileis-automated=falsewill effectively be a no-op.Signed-off-by: Paweł Gronowski [email protected]
- How to verify it
CI
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)