Bump k8s.io/* deps to 1.28
#1920
Conversation
afritzler
force-pushed
the
enh/bump-k8s-deps
branch
from
fb5da1a to
90dfe7d
Compare
There was a problem hiding this comment.
Thank you for updating this @afritzler!
/assign @kubeflow/wg-training-leads @tenzen-y
Makefile
Outdated
| @@ -62,7 +62,7 @@ ifeq ($(GOLANGCI_LINT),) | |||
| endif | |||
| golangci-lint run --timeout 5m --go 1.20 ./... | |||
|
|
|||
| ENVTEST_K8S_VERSION ?= 1.27 | |||
| ENVTEST_K8S_VERSION ?= 1.28 | |||
There was a problem hiding this comment.
Do we want to use 1.28 or 1.27 k8s version since our E2Es are set to run on 1.25, 1.26, and 1.27 version: https://github.com/kubeflow/training-operator/blob/master/.github/workflows/integration-tests.yaml#L50 ?
There was a problem hiding this comment.
Fair enough I reverted it back to 1.27.
| k8s.io/klog/v2 v2.90.1 | ||
| k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f | ||
| k8s.io/klog/v2 v2.100.1 | ||
| k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 |
There was a problem hiding this comment.
I am wondering do we really need separate go.mod file for Swagger ?
There was a problem hiding this comment.
I actually didn't plan to touch this part as I wanted to keep the structure as is.
cmd/training-operator.v1/main.go
Outdated
| flag.IntVar(&monitoringPort, "monitoring-port", 9443, "Endpoint port for displaying monitoring metrics. "+ | ||
| "It can be set to \"0\" to disable the metrics serving.") | ||
| flag.IntVar(&webhookServerPort, "webhook-server-port", 9443, "Endpoint port for the webhook server.") |
There was a problem hiding this comment.
This is a bug and breaking change. So, can you open another PR?
There was a problem hiding this comment.
Reverted the flag to the old one.
There was a problem hiding this comment.
Not sure that this is a bug, because the old Port field was actually the webhook server port and not the monitoring port. Anyways the behaviour should be the same again and needs to be addressed separately.
There was a problem hiding this comment.
Not sure that this is a bug, because the old Port field was actually the webhook server port and not the monitoring port.
Correct. I meant that removing the existing flag is breaking changes. So we should work on the breaking change at separate PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afritzler, tenzen-y 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 |
|
/hold |
|
@kubeflow/wg-training-leads Could you approve CI? |
|
Looks like some of the integration tests are running out of disk space in the GH runner. 🥴 |
@afritzler I fixed the issue in another PR. So can you rebase this PR? |
- Bump k8s.io/* deps to 1.28 - Fix metrics bind address assignment in manager setup - Rename metrics-port flag to webhook-server-port as it was wrongly used
- Revert envtest to 1.27 - Use generate-groups.sh instead of kube_codegen.sh
afritzler
force-pushed
the
enh/bump-k8s-deps
branch
from
d477faf to
ae040dd
Compare
Thx & done! |
|
/lgtm |
|
Oops. Before approving CI, this PS seems to been merged 😞 |
What this PR does / why we need it:
k8s.io/*deps to 1.28metrics-portflag towebhook-server-portas it was wrongly usedChecklist: