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

Set Created to 0001-01-01T00:00:00Z on older API versions #47374

Merged
merged 2 commits into from Feb 14, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Feb 12, 2024

This matches the prior behavior before 2a6ff3c.

This also updates the Swagger documentation for the current version to note that the field might be the empty string and what that means.

Fixes #47368

(if accepted, this should be backported to 25.x)

Changelog

- API: Populate a missing `Created` field in `GET /images/{id}/json` with `0001-01-01T00:00:00Z` for API version <= 1.43.

This matches the prior behavior before 2a6ff3c.

This also updates the Swagger documentation for the current version to note that the field might be the empty string and what that means.

Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
@tianon
Copy link
Member Author

tianon commented Feb 12, 2024

Restarted GHA for https://github.com/moby/moby/actions/runs/7877638978/job/21494256308?pr=47374#step:6:1902 which looks like it's probably just flaky 😄

=== Failed
=== FAIL: libnetwork/networkdb TestNetworkDBCRUDTableEntries (7.60s)
    networkdb_test.go:309: Entry existence verification test failed for node2(a349cda6da8c)

@tianon
Copy link
Member Author

tianon commented Feb 12, 2024

/ # docker image inspect -f '{{ .Created }}' hivemq/hivemq-swarm:4.25.0

/ # DOCKER_API_VERSION=1.44 docker image inspect -f '{{ .Created }}' hivemq/hivemq-swarm:4.25.0

/ # DOCKER_API_VERSION=1.43 docker image inspect -f '{{ .Created }}' hivemq/hivemq-swarm:4.25.0
0001-01-01T00:00:00Z

Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

Would be also nice to address the wrong formatting in image list (could be in a follow up):
3. docker image list
=> CREATED = 54 years ago

@@ -1746,7 +1746,7 @@ definitions:
Created:
description: |
Date and time at which the image was created, formatted in
[RFC 3339](https://www.ietf.org/rfc/rfc3339.txt) format with nano-seconds.
[RFC 3339](https://www.ietf.org/rfc/rfc3339.txt) format with nano-seconds, or empty if the field was not set in the image config.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also note this change in version-history.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh, good catch, I'll update (but I might not get to it today, so any other maintainer should feel free to take over 😅).

@thaJeztah
Copy link
Member

Yeah, this is probably an OK stop-gap solution. I agree with the discussion on the ticket that we should consider making this an omitempty though (in future?).

@tianon
Copy link
Member Author

tianon commented Feb 14, 2024

I agree with the discussion on the ticket that we should consider making this an omitempty though (in future?).

Agreed! My first priority was addressing the backwards compatibility break for 25.x, and it seemed reasonable to decide it was "intentional" for 25.x and consider something more in 26.x (such as omitempty), but I do not feel strongly about changing this to delay committing to an actual break in behavior here until 26.x and applying my time.Time{} hack to 25.x's API version too so we can do both breaks intentionally at the same time in 26.x instead (ie, no "empty" intermediate state and go straight to omitempty in 26.x).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@neersighted
Copy link
Member

Backport at #47387

@tianon tianon deleted the api-inspect-created branch February 14, 2024 18:16
@tianon
Copy link
Member Author

tianon commented Feb 14, 2024

Oh, can we apply omitempty conditionally on API version the future? If not, this might be our only chance to add it. 😅 🤦

@thaJeztah
Copy link
Member

Now that we always set it for old API versions, I guess omitempty would be just.. that? We already shipped this API version so I didn't want to push too hard, but yeah, would love to have it

@tianon
Copy link
Member Author

tianon commented Feb 14, 2024

Well we could also revert/change this, apply the fix for 25.x API version too and add omitempty there.

@neersighted
Copy link
Member

I think we can just add omitempty and keep this change; that will mean that API 1.44 will have no empty string and older API versions will have the bogus timestamp.

@tianon
Copy link
Member Author

tianon commented Feb 23, 2024

Did we get to that follow up, or is that still pending? 👀

@Subbaraman-Sowmya
Copy link

It is still pending ..not able to see the api-inspect-created

@neersighted
Copy link
Member

@tianon I wasn't sure if we reached any consensus on that being a good idea; I can open a PR making that change.

@tianon
Copy link
Member Author

tianon commented Feb 23, 2024

I think if we're going to do it, we probably have to quickly -- maybe a PR is the best way to figure out if we have consensus? 😄

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.

"Created" in image inspect response is not a valid date/time if "created" is absent in the image config
5 participants