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

Rename sandbox to podsandbox. #222

Merged

Conversation

Random-Liu
Copy link
Contributor

@Random-Liu Random-Liu commented Jan 11, 2018

This is discussed in sig-node. I sent the PR here to carry on the discussion.

The rule I used in this PR:

  1. In command s -> p, and sandbox -> pod;
  2. In usage SANDBOX -> PODSANDBOX;
  3. sandbox-config.yaml -> podsandbox-config.yaml;
  4. In flag --sandbox -> --podsandbox
  5. In command description 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 lantaol@google.com

Signed-off-by: Lantao Liu <lantaol@google.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2018
@yujuhong
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 Jan 11, 2018
@@ -147,8 +147,8 @@ var podSandboxStatusCommand = cli.Command{
}

var listPodSandboxCommand = cli.Command{
Name: "sandboxes",
Usage: "List sandboxes",
Name: "pods",
Copy link
Contributor

Choose a reason for hiding this comment

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

pods -> podsandboxes ?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

pods looks more simple

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense. :-)

Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

LGTM

@feiskyer feiskyer merged commit 0cb5a14 into kubernetes-sigs:master Jan 11, 2018
@Random-Liu Random-Liu deleted the rename-sandbox-to-podsandbox branch January 11, 2018 02:33
@Random-Liu Random-Liu added this to the v1.0.0-beta.0 milestone Jan 11, 2018
Copy link
Contributor

@abhi abhi left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow
Copy link
Contributor

@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.

@Random-Liu
Copy link
Contributor Author

@mikebrow I think at CRI level pod = pod sandbox + containers, so we may not want to call it a pod.

However, sandbox is new to user, so we call it pod sandbox to make it a bit more straight forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants