search

kubeflow / common

Common APIs and libraries shared by other Kubeflow operator repositories.
Apache License 2.0
51 stars 73 forks source link

Create expectation package #74

Closed Jeffwan closed 4 years ago

Jeffwan commented 4 years ago

Resolve #48. Besides #73, this is the last PR to remove direct Kubernetes dependency. I think current strategy is folk PodControlInterface and Expectations from main Kubernetes pkg and maintain in common repo. This also helps to make sure our code is reliable without worrying about changes K8s version upgrade brings.

For long term, I will try to brings these changes to kubernetes/client-go and we can use client-go directly. Track progress here. https://github.com/kubernetes/client-go/issues/332

The purpose of this PR is to get ride of direct Kubernetes dependency. Expectation is being used in JobController to tell the number of pods/services they expect.

In the future, Instead of import k8s.io/kubernetes/pkg/controller, User need to import github.com/kubeflow/common/pkg/controller.v1/expectation and use expectation.NewControllerExpectations instead.

Signed-off-by: Jiaxin Shan seedjeffwan@gmail.com

/cc @gaocegege @terrytangyuan

kubeflow-bot commented 4 years ago

This change is Reviewable

Jeffwan commented 4 years ago 
pkg/controller.v1/expectation/expectation_test.go:87:19: Error return value of `e.SetExpectations` is not checked (errcheck)
638 e.SetExpectations(rcKey, adds, dels)
639                  ^
640pkg/controller.v1/expectation/expectation_test.go:123:19: Error return value of `e.SetExpectations` is not checked (errcheck)
641 e.SetExpectations(rcKey, 1, 2)
642                  ^
643The command "golangci-lint run ./..." exited with 1.

This is too restrict.. it's pretty common to ignore output check. Disable errcheck for this.

Jeffwan commented 4 years ago

/hold

Please help on review first. I will run some real test against this version manually.

Jeffwan commented 4 years ago

@gaocegege This one bring expectation back to repo. We remove all the direct dependency of Kubernetes now.

terrytangyuan commented 4 years ago

Is this still on hold?

Jeffwan commented 4 years ago

/hold cancel

This can be merged. @terrytangyuan

k8s-ci-robot commented 4 years ago

[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: - ~~[OWNERS](https://github.com/kubeflow/common/blob/master/OWNERS)~~ [terrytangyuan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment