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.

Sign up for GitHub

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

Jump to bottom

Upgrade Kubernetes dependencies #502

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Upgrade Kubernetes dependencies #502

merged 1 commit into from
Jan 26, 2023

Conversation

tenzen-y
Copy link
Member

Signed-off-by: Yuki Iwai [email protected]

I upgraded Kubernetes dependencies.

Fixes: #483

alculquicondor
alculquicondor reviewed Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

Surprisingly small change. Nice!

Makefile
KUBECTL_VERSION=v1.21.4

KUBECTL_VERSION=v1.25.5
ENVTEST_K8S_VERSION=1.25.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

no newer patch version available?

Copy link
Member Author

@tenzen-y tenzen-y Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that is the newest patch version:

$ ./bin/setup-envtest list --arch "*" --os "*" | grep 25
(installed)  v1.25.0  darwin/arm64
(available)  v1.25.0  darwin/amd64
(available)  v1.25.0  darwin/arm64
(available)  v1.25.0  linux/amd64
(available)  v1.25.0  linux/arm64
(available)  v1.25.0  linux/ppc64le

hack/tools.go
// See the License for the specific language governing permissions and
// limitations under the License.

package tools
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I referred to the following:

https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

@alculquicondor
Copy link
Collaborator

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 13, 2023
@tenzen-y
Copy link
Member Author

Thanks for the review!

/assign @terrytangyuan

@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from cd2c860 to 5b28de0 Compare January 13, 2023 21:15
@google-oss-prow google-oss-prow bot removed the lgtm label Jan 13, 2023
@tenzen-y
Copy link
Member Author

@alculquicondor I forgot to re-generate go client codes. PTAL

@tenzen-y
Copy link
Member Author

Something is wrong...

@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from 5b28de0 to d93642d Compare January 13, 2023 21:19
@tenzen-y tenzen-y changed the title Upgrade Kubernetes dependencies [WIP] Upgrade Kubernetes dependencies Jan 13, 2023
This was referenced Jan 17, 2023
Closed
Open
@tenzen-y
Copy link
Member Author

@alculquicondor The code-generator seems unable to generate client codes since we have a different Go module name, github.com/kubeflow/mpi-operator/v2, and directory structure, github.com/kubeflow/mpi-operator.

So we need to change the Go module name to github.com/kubeflow/mpi-operator or move all codes to the directory github.com/kubeflow/mpi-operator/v2.

The latter means we revert #496.

WDYT?

@alculquicondor
Copy link
Collaborator

Interesting. I guess that's not a problem in k8s.io/klog.

I'm fine with changing the module name. I wouldn't expect users of the internal packages, right?

@tenzen-y
Copy link
Member Author

Interesting. I guess that's not a problem in k8s.io/klog.

As I can see, the k8s.io/klog does not generate the k8s client code by code-generator. So k8s.io/klog does not face the same issue.

I'm fine with changing the module name. I wouldn't expect users of the internal packages, right?

Yes, I think so too.

@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch 2 times, most recently from 1bd225c to 260ed2e Compare January 25, 2023 14:53
@alculquicondor
Copy link
Collaborator

Consider splitting the module rename in a separate PR.

@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from 260ed2e to 1920819 Compare January 25, 2023 14:54
@tenzen-y
Copy link
Member Author

Consider splitting the module rename in a separate PR.

Makes sense.

@tenzen-y tenzen-y mentioned this pull request Jan 25, 2023
Merged
@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from 1920819 to 1b924ec Compare January 25, 2023 17:44
@tenzen-y tenzen-y changed the title [WIP] Upgrade Kubernetes dependencies Upgrade Kubernetes dependencies Jan 25, 2023
@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from 1b924ec to 9e027bb Compare January 25, 2023 18:06
@tenzen-y
Copy link
Member Author

@alculquicondor I have rebased and squashed. PTAL.

alculquicondor
alculquicondor reviewed Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

Minor questions

Makefile
@@ -54,8 +74,10 @@ dev_manifest:

.PHONY: generate
generate:
go generate ./pkg/... ./cmd/... && \
openapi-gen --input-dirs github.com/kubeflow/mpi-operator/pkg/apis/kubeflow/v2beta1,k8s.io/api/core/v1,k8s.io/apimachinery/pkg/apis/meta/v1,k8s.io/apimachinery/pkg/api/resource,k8s.io/apimachinery/pkg/runtime,k8s.io/apimachinery/pkg/util/intstr,k8s.io/apimachinery/pkg/version,github.com/kubeflow/common/pkg/apis/common/v1 --output-package github.com/kubeflow/mpi-operator/pkg/apis/kubeflow/v2beta1 --go-header-file hack/boilerplate/boilerplate.go.txt
go generate ./pkg/... ./cmd/...
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this changing?

Copy link
Member Author

Choose a reason for hiding this comment

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

L58 duplicates the below:

mpi-operator/hack/python-sdk/gen-sdk.sh

Lines 34 to 35 in cd83424

echo "Generating V2 OpenAPI specification ..."
openapi-gen --input-dirs github.com/kubeflow/mpi-operator/pkg/apis/kubeflow/v2beta1,k8s.io/api/core/v1,k8s.io/apimachinery/pkg/apis/meta/v1,k8s.io/apimachinery/pkg/api/resource,k8s.io/apimachinery/pkg/runtime,k8s.io/apimachinery/pkg/util/intstr,k8s.io/apimachinery/pkg/version,github.com/kubeflow/common/pkg/apis/common/v1 --output-package github.com/kubeflow/mpi-operator/pkg/apis/kubeflow/v2beta1 --go-header-file hack/boilerplate/boilerplate.go.txt

So I removed L58.

@alculquicondor
Copy link
Collaborator

Please squash.
LGTM otherwise

@tenzen-y tenzen-y force-pushed the upgrade-k8s-dependencies branch from a2d636b to 40dbddd Compare January 25, 2023 18:31
@tenzen-y
Copy link
Member Author

@alculquicondor Squashed.

@alculquicondor
Copy link
Collaborator

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 25, 2023
@tenzen-y
Copy link
Member Author

/assign @terrytangyuan

terrytangyuan
terrytangyuan approved these changes Jan 26, 2023
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.

/lgtm
/approve

@google-oss-prow
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-prow google-oss-prow bot merged commit 05ac6ad into kubeflow:master Jan 26, 2023
@tenzen-y tenzen-y deleted the upgrade-k8s-dependencies branch January 26, 2023 18:28
@tenzen-y tenzen-y mentioned this pull request Jan 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@gaocegege gaocegege Awaiting requested review from gaocegege

Assignees

@alculquicondor alculquicondor

@terrytangyuan terrytangyuan

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

Successfully merging this pull request may close these issues.

Mismatch kube-open api refs
3 participants
@tenzen-y @alculquicondor @terrytangyuan