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

kubelet: Refactor prober. #7009

Merged
merged 1 commit into from Apr 20, 2015
Merged

kubelet: Refactor prober. #7009

merged 1 commit into from Apr 20, 2015

Conversation

yifan-gu
Copy link
Contributor

Decompose the health check prober from the kubelet.

@yujuhong @dchen1107 @vmarmol

Decompose the health check prober from the kubelet.

// NewProber creates a Prober, it takes a command runner and
// several container info managers.
func NewProber(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect the function to be called outside of this package? Why capitalized the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yujuhong Yes, I am expecting to move it to kubelet/prober, otherwise container runtime cannot import it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or move it to kubelet/container...

Actually this function does feel to belong to kubelet in some sense... Hopefully if kubelet someday does not import dockertools or rkt directly, we can move them back...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way to work around this is to define an interface in kubelet/container:

type prober interface {
    Probe()
}

And then we can pass the kubelet as this interface to the runtime without the circular import problem.

But I feel it's cleaner to just move it to kubelet/prober. What's your opinion?

@yujuhong
Copy link
Contributor

I am fine with it being in kubelet/prober :)

yujuhong added a commit that referenced this pull request Apr 20, 2015
@yujuhong yujuhong merged commit cd61aa9 into kubernetes:master Apr 20, 2015
@yifan-gu yifan-gu deleted the kube_dep branch May 7, 2015 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants