-
Notifications
You must be signed in to change notification settings - Fork 456
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
Rename sandbox
to podsandbox
.
#222
Rename sandbox
to podsandbox
.
#222
Conversation
Signed-off-by: Lantao Liu <[email protected]>
/lgtm |
@@ -147,8 +147,8 @@ var podSandboxStatusCommand = cli.Command{ | |||
} | |||
|
|||
var listPodSandboxCommand = cli.Command{ | |||
Name: "sandboxes", | |||
Usage: "List sandboxes", | |||
Name: "pods", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pods
-> podsandboxes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command will be frequently used, I feel like podsandboxes
is too long, which will be very annoying in the future. :P
That's why I use crictl pods
for this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pods
looks more simple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Random-Liu I was expecting full replacement of sandbox for pod, not sandbox to pod sandbox in some cases? Might be wrong but I thought the point was only internal developers of kublet know what sandbox refers to. |
@mikebrow I think at CRI level pod = However, |
This is discussed in sig-node. I sent the PR here to carry on the discussion.
The rule I used in this PR:
s
->p
, andsandbox
->pod
;SANDBOX
->PODSANDBOX
;sandbox-config.yaml
->podsandbox-config.yaml
;--sandbox
->--podsandbox
sandbox
->pod sandbox
.Note: This is a breaking change. We may want to emphasize this in release note. And after upgrade, we need to change all places which use
crictl
, including cri-containerd, cri-o, kubeadm etc.@yujuhong @feiskyer @runcom @mrunalp @mikebrow @abhi @yanxuean @miaoyq @dchen1107
Signed-off-by: Lantao Liu [email protected]