Skip to content
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

docs/deprecated: Deprecate Container fields in image inspect #4893

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Feb 22, 2024

See moby/moby#46939

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@@ -110,6 +111,19 @@ The table below provides an overview of the current status of deprecated feature
| Removed | [`--run` flag on `docker commit`](#--run-flag-on-docker-commit) | v0.10 | v1.13 |
| Removed | [Three arguments form in `docker import`](#three-arguments-form-in-docker-import) | v0.6.7 | v1.12 |

### `Container` and `ContainerConfig` fields in Image inspect

**Deprecated in Release: v25.0**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we forget mentioning these as deprecated for the v25 release? Wondering if that means we need to wait for v27.0 to remove them, or did we document it in the API changes at the time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we forgot to put it there, but we mentioned it in version-history.md 🙈

The Container and ContainerConfig fields in the GET /images/{name}/json response are deprecated and will no longer be included in API v1.45.

But I'm fine with waiting for v27.0 for the removal, especially since the docker-py with the fixed tests is not released yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arf, I somehow missed your last comments. Some thoughts (maybe we don't have to postpone the removal);

  • The API changes document is the "canonical" place for changes in the engine API, including deprecations
  • I thought we always included a mention like "for changes and deprecations in the API refer to <link to API changes>"; if we don't, we should fix that.
  • ☝️ perhaps we should also do a cross-reference in this deprecated page, to do the same ("for an overview of changes in API ...")

That said; I don't think it necessarily hurts to call out / repeat important changes here, but we may not have to for all of them.

@vvoland @dvdksn WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW; for some of these, it would at least make things easier as well, because the API is versioned so if you use the "older API version", nothing changes, but yeah, in some cases that may affect output of docker CLI commands.

docker inspect was meant to show the underlying "low-level" information, which would still be "accurate" here, but that underlying information now may no longer have these fields (or not set).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We link to both the deprecated.md and version-history.md from the release note:

image

I don't think it hurts to also cross-reference between these two.

If version-history.md is canonical then we should be safe to remove in 26.0 since this was called out (and linked from the release note). It just wasn't called out everywhere.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the removal notice to v26.

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2024

Codecov Report

Merging #4893 (02ee6b0) into master (f2e98f9) will increase coverage by 0.00%.
Report is 47 commits behind head on master.
The diff coverage is n/a.

❗ Current head 02ee6b0 differs from pull request most recent head 3b66d84. Consider uploading reports for the commit 3b66d84 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4893   +/-   ##
=======================================
  Coverage   61.31%   61.31%           
=======================================
  Files         287      287           
  Lines       20063    20065    +2     
=======================================
+ Hits        12301    12303    +2     
- Misses       6868     6869    +1     
+ Partials      894      893    -1     

@vvoland
Copy link
Collaborator Author

vvoland commented Feb 28, 2024

Changed deprecation candidate to v27

@vvoland vvoland modified the milestones: 26.0.0, 27.0.0 Feb 28, 2024
@vvoland vvoland mentioned this pull request Mar 5, 2024
1 task
Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nits, nonblocking

docs/deprecated.md Outdated Show resolved Hide resolved
@vvoland vvoland modified the milestones: 27.0.0, 26.0.0 Mar 14, 2024
See moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland merged commit 4eef4af into docker:master Mar 18, 2024
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants