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.

Sign up for GitHub

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

Jump to bottom

Fix container/sandbox list. #217

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Fix container/sandbox list. #217

merged 1 commit into from
Dec 14, 2017

Conversation

Random-Liu
Copy link
Contributor

For #179.

This PR:

  1. Added -l and -n in crictl sandboxes.
  2. Fix container list order, latest created container should be on top.
  3. Change sandbox to sort by created timestamp, too. @yanxuean Are you ok with that? If user want to know sandboxes in the same namespace, they could always use --namespace to filter.

Signed-off-by: Lantao Liu [email protected]

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 13, 2017
feiskyer
feiskyer reviewed Dec 13, 2017
View reviewed changes
cmd/crictl/sandbox.go
cli.IntFlag{
Name: "last, n",
Usage: "Show last n recently created sandboxes",
},
Copy link
Member

Choose a reason for hiding this comment

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

Seems two options are same. Merge last to latest and defaulting latest to one?

Copy link
Contributor Author

@Random-Liu Random-Liu Dec 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Initially just want to keep the same UX with docker, but of course we could make change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feiskyer I tried to merge them into one flag. But I find that it's hard to set default for -l, because once you define it as an int flag, you have to specify a value when using it.

Given that I think it's fine to keep 2 flags as what docker does.

Copy link
Member

Choose a reason for hiding this comment

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

OK, didn't notice this problem. let's keep both

@Random-Liu Random-Liu force-pushed the fix-sandbox-container-list branch from ef9890a to 0360805 Compare December 13, 2017 03:24
yanxuean
yanxuean reviewed Dec 13, 2017
View reviewed changes
cmd/crictl/container.go Outdated
@@ -590,8 +590,8 @@ func ListContainers(client pb.RuntimeServiceClient, opts listOptions) error {
if err != nil {
return err
}
sort.Sort(containerByCreated(r.GetContainers()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think getContainersList should include sort.Sort inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is more completer in logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yanxuean
Copy link
Contributor

Change sandbox to sort by created timestamp, too. @yanxuean Are you ok with that? If user want to know sandboxes in the same namespace, they could always use --namespace to filter.

I am OK.

yanxuean
yanxuean reviewed Dec 13, 2017
View reviewed changes
cmd/crictl/sandbox.go
@@ -172,12 +166,12 @@ var listPodSandboxCommand = cli.Command{
Usage: "filter by pod sandbox namespace",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add shortname for namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

last is using the shortname n.

yanxuean
yanxuean reviewed Dec 13, 2017
View reviewed changes
cmd/crictl/sandbox.go Outdated
@@ -396,7 +400,8 @@ func ListPodSandboxes(client pb.RuntimeServiceClient, opts listOptions) error {
if err != nil {
return err
}
sort.Sort(sandboxBySort(r.Items))
sort.Sort(sandboxByCreated(r.GetItems()))
r.Items = getSandboxesList(r.GetItems(), opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Random-Liu Random-Liu force-pushed the fix-sandbox-container-list branch from 0360805 to cd98da1 Compare December 13, 2017 18:41
@yanxuean
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2017
@Random-Liu Random-Liu merged commit 2c29c5c into kubernetes-sigs:master Dec 14, 2017
@Random-Liu Random-Liu deleted the fix-sandbox-container-list branch December 14, 2017 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@yanxuean yanxuean yanxuean left review comments

Assignees

@feiskyer feiskyer

@yanxuean yanxuean

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Random-Liu @yanxuean @feiskyer @k8s-ci-robot