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

refactor: extract the printing logic out of pkg for container list #2333

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

ningziwen
Copy link
Contributor

@ningziwen ningziwen commented Jun 27, 2023

And in the community, people may need to add some features of their own, like #1631, so it's time to refactor the command package!

Based on the issue, one of the reason of refactoring nerdctl is to make it easy for developers to import subcommand package. The current refactoring pattern of container list mixed printing logic in List() in pkg, which makes it not flexible to be imported to use. The printing logic is very specific to nerdctl cmd layer, so the idea is to moving the printing logic out of pkg, and let List() return a generic container list.

This is not the only solution to unblock the use case. The alternative is to simply make filterContainers and other functions public.

@ningziwen ningziwen marked this pull request as ready for review June 28, 2023 07:27
ID string
Image string
Platform string // nerdctl extension
Names string
Names []string
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The general principle I'm following for this PR, is to make the pkg layer's data type reflect the intrinsic nature, generic without impacting by the printing preference.

One example to demonstrate this principle is I changed CreatedAt from string to `time.Time.

For "Names", the naming is a bit confusing because each container can only have one name.

But I looked into the Docker API spec and found Names is Array in /containers/json response. That made me think it is not just confusing naming but there may be some design consideration behind the scene to keep the flexibility of supporting multiple names in the future.

So I tend to think []string can reflect more intrinsic nature. However, the evidences are not that strong so I don't have strong preference. Happy to ignore the naming and make it completely string if you prefer it.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 21f4c6e into containerd:main Jun 29, 2023
21 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.

2 participants