-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
pkg/kubelet/dockertools/docker_manager.go: removing unused stuff #40527
pkg/kubelet/dockertools/docker_manager.go: removing unused stuff #40527
Conversation
Hi @php-coder. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@@ -196,18 +196,6 @@ type podGetter interface { | |||
GetPodByUID(kubetypes.UID) (*v1.Pod, bool) | |||
} | |||
|
|||
func PodInfraContainerEnv(env map[string]string) kubecontainer.Option { |
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.
Mesos relies on this function, see: https://github.com/kubernetes-incubator/kube-mesos-framework/blob/fd9531a578c6c46daaa1e01621efa7e8f4dfe501/pkg/executor/service/service.go#L243
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.
Thanks for the info. I've removed this commit.
ErrNoContainersInPod = errors.New("NoContainersInPod") | ||
|
||
// ErrNoPodInfraContainerInPod is returned when there is no pod infra container for a given pod | ||
ErrNoPodInfraContainerInPod = errors.New("NoPodInfraContainerInPod") |
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.
@yujuhong ok to remove these error reasons, or were you planning on using them?
Seemed usage of ErrNoContainersInPod
stopped in bb417e8 -- pkg/kubelet/dockertools/manager.go
and ErrNoPodInfraContainerInPod
doesn't look like it was ever in use as far as I can tell (note I haven't searched back before the origin of pkg/kubelet/dockertools/manager.go
).
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.
Don't think we are going to use them, especially now that we are transitioning to CRI. Most of the code in this package (especially those mentioning "infra container") will be deprecated eventually.
@php-coder If there is no release note, you can just omit that entire section from the initial comment. |
9f23bc0
to
c52d367
Compare
/lgtm Thanks for this work :) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: Random-Liu, php-coder Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 40527, 40738, 39366, 40609, 40748) |
Automatic merge from submit-queue |
This PR removes unused constants and variables. I checked that neither kubernetes nor openshift code aren't using them.