search

kubeflow / common

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

Bump k8s dependency to 1.19 and add context support #126

Closed Jeffwan closed 3 years ago

Jeffwan commented 3 years ago
  1. Upgrade k8s dependency to 1.19
  2. 1.18 client-go add context and extra options. Adapt to the new changes.
  3. Upgrade golang to 1.15 to match golang used in 1.19

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

Jeffwan commented 3 years ago

/hold

Jeffwan commented 3 years ago

/cc @Thor-wl @shinytang6 for extra review due to blocked PR. https://github.com/kubeflow/common/pull/123 /cc @kubeflow/wg-training-leads

google-oss-robot commented 3 years ago

@Jeffwan: GitHub didn't allow me to request PR reviews from the following users: Thor-wl, shinytang6.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/kubeflow/common/pull/126#issuecomment-809900220): >/cc @Thor-wl @shinytang6 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
gaocegege commented 3 years ago

I think the main purpose of https://github.com/kubeflow/common/pull/123 is to avoid the direct dependency to k8s. Then I am wondering if we should upgrade the version to 1.19.

Do you want to add context first, then we can eliminate the k8s import packages?

Thor-wl commented 3 years ago

I'm both OK for avoiding the direct depend dependency to k8s first or upgrade to k8s to v1.19 first. You can make a decision @Jeffwan @gaocegege

Jeffwan commented 3 years ago

I think the main purpose of #123 is to avoid the direct dependency to k8s. Then I am wondering if we should upgrade the version to 1.19.

Do you want to add context first, then we can eliminate the k8s import packages?

@gaocegege I think your concern is reasonable. community is releasing 1.21. It's better to keep < 3 version changes with latest version and we will update to higher version sometime soon.

1.18 brings in context changes, I am ok to bump to 1.18. The major reason I choose 1.19 is because volcano/api now is using 1.19 https://github.com/volcano-sh/apis/blob/cfacea273e7da6b53170cc00ece9b2caf019316f/go.mod#L6-L10

Another possible solution is to ask volcano/api to downgrade to version >=1.16 <=1.17 and we don't need client-go related changes. But we will still need to upgrade dependencies later.

Thor-wl commented 3 years ago

I think the main purpose of #123 is to avoid the direct dependency to k8s. Then I am wondering if we should upgrade the version to 1.19. Do you want to add context first, then we can eliminate the k8s import packages?

@gaocegege I think your concern is reasonable. community is releasing 1.21. It's better to keep < 3 version changes with latest version and we will update to higher version sometime soon.

1.18 brings in context changes, I am ok to bump to 1.18. The major reason I choose 1.19 is because volcano/api now is using 1.19 https://github.com/volcano-sh/apis/blob/cfacea273e7da6b53170cc00ece9b2caf019316f/go.mod#L6-L10

Another possible solution is to ask volcano/api to downgrade to 1.17 and we don't need client-go related changes. But we will still need to upgrade dependencies later.

It's OK for me to provide volcano.sh/apis release-1.17 to match k8s.io v1.17. Look forward to your decision.

Jeffwan commented 3 years ago

volcano.sh/apis v1.2.0-k8s1.16.15 this makes sense. I think @gaocegege will like it. :D In this case, we will not bump k8s version at this moment, this PR can be left for future usage.

gaocegege commented 3 years ago

SGTM! I think we will update to a higher version soon.

Thor-wl commented 3 years ago

volcano.sh/apis v1.2.0-k8s1.16.15 this makes sense. I think @gaocegege will like it. :D In this case, we will not bump k8s version at this moment, this PR can be left for future usage.

SGTM! I think we will update to a higher version soon.

@Jeffwan @gaocegege So what's the version shall i provide?

Jeffwan commented 3 years ago

/cc @kubeflow/wg-training-leads /hold cancel

This unblocks user's request @chxk in https://github.com/kubeflow/common/issues/132

google-oss-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, Jeffwan, 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)~~ [Jeffwan,terrytangyuan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment