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

fix the bugs due to refactoring code #776

Closed
wants to merge 3 commits into from
Closed

fix the bugs due to refactoring code #776

wants to merge 3 commits into from

Conversation

ChanYiLin
Copy link
Member

@ChanYiLin ChanYiLin commented Aug 12, 2018

Some functions which originally belonged to TFJobController are now changed to JobController when last time refactoring the code #767 .

This is also the root cause for issue #774


This change is Reviewable

@k8s-ci-robot
Copy link

Hi @ChanYiLin. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@ChanYiLin
Copy link
Member Author

/assign @gaocegege
Would you help me to have a look?
Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 57.957% when pulling 7817026 on ChanYiLin:master into 44c5382 on kubeflow:master.

@coveralls
Copy link

coveralls commented Aug 12, 2018

Coverage Status

Coverage increased (+0.07%) to 58.026% when pulling 62ae492 on ChanYiLin:master into 44c5382 on kubeflow:master.

@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: gaocegege

If they are not already assigned, you can assign the PR to them by writing /assign @gaocegege 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

@ChanYiLin ChanYiLin changed the title fix the bugs of calling sycnPDB and deletePDB function fix the bugs due to refactoring code Aug 12, 2018
Copy link
Member

@ScorpioCPH ScorpioCPH left a comment

Choose a reason for hiding this comment

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

LGTM.

@ScorpioCPH
Copy link
Member

After taking a second look, TFJobController has embedded field jobcontroller.JobController as defined here, so i think it is ok now.

But i'm just wondering why not rename TFJobController to TFController as we have renamed the folder to tfcontroller, and move controller_pod/service/status.go files into jobcontroller folder.

FYI @johnugeorge @gaocegege

@ChanYiLin
Copy link
Member Author

Oh, its my fault, I got it. lol
we can just close this PR, thanks

@johnugeorge
Copy link
Member

@ChanYiLin I didn't understand this PR. Did you find any issues?

@johnugeorge
Copy link
Member

@ScorpioCPH

Agree with you regarding the renaming. I will raise a PR for it

Wrt to moving controller_pod/service/status.go files into jobcontroller folder , I found that most of the code in status.go is specific to v1alpha2.TFJob and hence kept it in TFController folder. Do you have other ideas?

@ChanYiLin
Copy link
Member Author

@johnugeorge
I opened this issue because I was reading the source code and saw the issue #774 .
My original idea was that because of the refactoring code, some functions like GetPodsForJob() was moved to jobController and thus,
pods, err := tc.GetPodsForJob(tfjob) in tfcontroller.go L336 should be
tc.JobController .GetPodsForJob(tfjob)

but I was wrong at the end, since we've already embedded jobcontroller.JobController in TFJobController type struct, so this issue is not really a problem.

I suggest we can close this issue, and open another issue to discuss the naming problem

cc @ScorpioCPH

@johnugeorge
Copy link
Member

Closing PR as changes are not required
/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants