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

Revert #135: change rtype to commonv1.ReplicaType #158

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Revert #135: change rtype to commonv1.ReplicaType #158

merged 1 commit into from
Aug 30, 2021

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented Aug 30, 2021 •
edited
Loading

#135 This change brings some extra side effects and make training operator dependency upgrade fail. Since this change is kind of refactor, we determine to revert it at this moment.

/cc @gaocegege @terrytangyuan @kubeflow/wg-training-leads

Need two of you to approve the change.

I kick off an end to end test in tf-operator to verify this is working. kubeflow/training-operator#1395

This PR unblocks kubeflow/training-operator#1388 and kubeflow/training-operator#1383

@Jeffwan
Revert kubeflow#135: change rtype to commonv1.ReplicaType
7125957 
kubeflow#135 This change brings some extra side effects and make training operator dependency upgrade fail. Since this change is kind of refactor, we determine to revert it at this moment.
@Jeffwan
Copy link
Member Author

Jeffwan commented Aug 30, 2021

no build issues now.

➜  common git:(revert_rtype_change) ✗ go build ./...
➜  common git:(revert_rtype_change) ✗ go test ./...
?   	github.com/kubeflow/common/pkg/apis/common/v1	[no test files]
ok  	github.com/kubeflow/common/pkg/controller.v1/common	0.018s
ok  	github.com/kubeflow/common/pkg/controller.v1/control	0.033s
ok  	github.com/kubeflow/common/pkg/controller.v1/expectation	0.023s
ok  	github.com/kubeflow/common/pkg/util	0.020s
?   	github.com/kubeflow/common/pkg/util/k8sutil	[no test files]
?   	github.com/kubeflow/common/pkg/util/signals	[no test files]
ok  	github.com/kubeflow/common/pkg/util/train	0.013s
?   	github.com/kubeflow/common/test_job/apis/test_job/v1	[no test files]
?   	github.com/kubeflow/common/test_job/client/clientset/versioned	[no test files]
?   	github.com/kubeflow/common/test_job/client/clientset/versioned/fake	[no test files]
?   	github.com/kubeflow/common/test_job/client/clientset/versioned/scheme	[no test files]
?   	github.com/kubeflow/common/test_job/client/clientset/versioned/typed/test_job/v1	[no test files]
?   	github.com/kubeflow/common/test_job/client/clientset/versioned/typed/test_job/v1/fake	[no test files]
?   	github.com/kubeflow/common/test_job/client/informers/externalversions	[no test files]
?   	github.com/kubeflow/common/test_job/client/informers/externalversions/internalinterfaces	[no test files]
?   	github.com/kubeflow/common/test_job/client/informers/externalversions/test_job	[no test files]
?   	github.com/kubeflow/common/test_job/client/informers/externalversions/test_job/v1	[no test files]
?   	github.com/kubeflow/common/test_job/client/listers/test_job/v1	[no test files]
?   	github.com/kubeflow/common/test_job/controller.v1/test_job	[no test files]
?   	github.com/kubeflow/common/test_job/test_util/v1	[no test files]

This was referenced Aug 30, 2021
Closed
Closed
terrytangyuan
terrytangyuan approved these changes Aug 30, 2021
View reviewed changes
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.

Thanks!

/lgtm

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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

@google-oss-robot google-oss-robot merged commit 654b666 into kubeflow:release-0.3 Aug 30, 2021
@Jeffwan Jeffwan deleted the revert_rtype_change branch August 31, 2021 03:36
@zw0610
Copy link
Member

zw0610 commented Sep 6, 2021 •
edited
Loading

@Jeffwan For the master branch, does it means we should still use commonv1.ReplicaType for argument type? I was updating the common repo to master branch in tf-operator to use the reconciler package, but it breaks SetClusterSpec method.

@Jeffwan Jeffwan mentioned this pull request Sep 19, 2021
Closed
gaocegege pushed a commit to gaocegege/common that referenced this pull request Nov 23, 2021
@Jeffwan @gaocegege
Revert kubeflow#135: change rtype to commonv1.ReplicaType (kubeflow#158)
def4f33 
kubeflow#135 This change brings some extra side effects and make training operator dependency upgrade fail. Since this change is kind of refactor, we determine to revert it at this moment.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@gaocegege gaocegege Awaiting requested review from gaocegege

Assignees

@terrytangyuan terrytangyuan

Labels
approved lgtm size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jeffwan @google-oss-robot @zw0610 @terrytangyuan