Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

set podInformer/serviceInformer in NewJobController #34

Closed
wants to merge 2 commits into from

Conversation

sperlingxx
Copy link
Member

@sperlingxx sperlingxx commented May 5, 2019

Binding podInformer and serviceInformer onto jobController when we initialize a jobController by calling NewJobController.


This change is Reviewable

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: terrytangyuan

If they are not already assigned, you can assign the PR to them by writing /assign @terrytangyuan in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sperlingxx
Copy link
Member Author

/assign @terrytangyuan

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels May 6, 2019
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

@jian-he
Copy link
Contributor

jian-he commented May 10, 2019

After #36 the usage of podLister/serviceLister are removed, do we still need this change ?
If projects use kube-builder to generate the skeleton code, then I think the raw podInformer/serviceInformer structure won't be needed too.

@gaocegege
Copy link
Member

We do not use kubebuilder now. Thus we need the informer to watch resources in apiserver. But I am not sure if listers are needed here.

@gaocegege
Copy link
Member

Did we decided to use kubebuilder in tf-operator and pytorch-operator? @jian-he

I am not aware of it.

@jian-he
Copy link
Contributor

jian-he commented May 13, 2019

Did we decided to use kubebuilder in tf-operator and pytorch-operator? @jian-he

no, we didn't. Just want to mention this scenario.

Yes, we still need to use informer etc. Just think should that reside in specific job controller's own implementation (the ones that require the informer structure) or still put it in the common API.

@gaocegege
Copy link
Member

@jian-he Oh gotcha. Yeah it is the problem. Personally, I prefer keeping it in specific job controller. Or we will introduce something useless for kubebuilder use cases.

@johnugeorge
Copy link
Member

agree with @gaocegege

@terrytangyuan
Copy link
Member

Agreed. Then I think we can close this. WDYT? @sperlingxx

@sperlingxx
Copy link
Member Author

Agreed. Then I think we can close this. WDYT? @sperlingxx

I close this now.

@sperlingxx sperlingxx closed this May 16, 2019
georgkaleido pushed a commit to georgkaleido/common that referenced this pull request Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants