Skip to content

api/types: move various types to api/types/(images|containers|swarm) #46483

Merged
thaJeztah merged 10 commits intomoby:masterfrom
thaJeztah:api_move_image_types
Oct 12, 2023
Merged

api/types: move various types to api/types/(images|containers|swarm) #46483
thaJeztah merged 10 commits intomoby:masterfrom
thaJeztah:api_move_image_types

Conversation

@thaJeztah
Copy link
Member

- 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)

@thaJeztah thaJeztah added area/api API status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 14, 2023
@thaJeztah thaJeztah requested a review from tianon as a code owner September 14, 2023 17:48
@thaJeztah
Copy link
Member Author

Looks like I never pushed this one (I have another one pending, but that one still needed some work around search and auth to prevent a circular import)

@thaJeztah thaJeztah marked this pull request as draft September 14, 2023 18:18
@thaJeztah thaJeztah marked this pull request as ready for review September 14, 2023 21:58
@thaJeztah thaJeztah force-pushed the api_move_image_types branch 4 times, most recently from cd3365f to 231484b Compare September 20, 2023 20:19
@thaJeztah thaJeztah force-pushed the api_move_image_types branch 2 times, most recently from 82d806a to 79d1f77 Compare October 11, 2023 15:47
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.

LGTM; although it's probably worth noting that we are starting to have too many image(s) modules 😄

"github.com/containerd/containerd/images"
"github.com/docker/docker/image"
"github.com/docker/docker/daemon/images"
imagetypes "github.com/docker/docker/api/types/image"

@thaJeztah
Copy link
Member Author

although it's probably worth noting that we starting to have too many image(s) modules 😄

OHMAN, yes, we do, and with the containerd-image-store integration that definitely became more apparent. We may be able to avoid some of these if we work on cleaner separation of API types and internal types, but I guess it's something we will have to deal with for some time.

Perhaps we should enforce aliases for some of these; same as we did for the containerd-errdefs package;

# Enforce alias to prevent it accidentally being used instead of our
# own errdefs package (or vice-versa).
- pkg: github.com/containerd/containerd/errdefs
alias: cerrdefs

@thaJeztah
Copy link
Member Author

oh! needs another rebase (due to the containerd logs changes)

@thaJeztah thaJeztah force-pushed the api_move_image_types branch from 79d1f77 to ebef4ef Compare October 12, 2023 09:30
GetNode(string) (types.Node, error)
UpdateNode(string, uint64, types.NodeSpec) error
ServiceLogs(context.Context, *backend.LogSelector, *types.ContainerLogsOptions) (<-chan *backend.LogMessage, error)
GetNodes(types.NodeListOptions) ([]swarm.Node, error)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's in another commit, shouldn't types.NodeListOptions also go to api/types/Swarm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I didn't get to those (nodes, tasks) yet. I think I was still somewhat looking if everything should be swarm or if we should consider some of those to be separate entities (service, node, task), but doing so may require some additional thinking.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

I have one question but LGTM otherwise

@thaJeztah thaJeztah merged commit 0a82696 into moby:master Oct 12, 2023
@thaJeztah thaJeztah deleted the api_move_image_types branch October 12, 2023 13:29
@thaJeztah thaJeztah added this to the 25.0.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api API kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants