chore: upgrade k8s code-generator to v0.33.1 #15521
Open
+1,585
−3,099
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
go-to-protobuf v0.33.1 uses full module paths for proto source paths, requiring --output-dir to route generated .pb.go files to the correct locations. The tool also writes temporary .proto files to and cleans them from package directories. Since `go list` returns module cache paths regardless of any local copies, we must make the module cache writable, then re-download the corrupted k8s.io modules after generation completes. The approach: 1. chmod module cache writable for k8s.io packages 2. Copy k8s.io to GOPATH/src for --proto-import (protoc finds .proto files) 3. Symlink argo-workflows in GOPATH/src so --output-dir routes .pb.go to project 4. Re-download k8s.io modules after, since go-to-protobuf deletes files from them Signed-off-by: Alan Clucas <[email protected]> https://claude.ai/code/session_01DtkvsHyL9aY9mR78yDti3Y
Joibel
force-pushed
the
claude/upgrade-protobuf-makefile-q7f9h
branch
from
67b95a0 to
fc87c1f
Compare
Signed-off-by: Alan Clucas <[email protected]>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughUpdates the code-generation workflow from v0.21.5 to v0.33.1 by modifying the Makefile with expanded proto generation steps, proto package paths, and generic typed client implementations across the clientset, informers, and listers packages. Systematically replaces REST-based clients with generic client wrappers and refactors lister implementations to use generic ResourceIndexer patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 480-498: The chmod -R +w on $(shell go env GOMODCACHE)/k8s.io
makes the entire k8s.io cache writable but only [email protected] and
[email protected] are re-downloaded, leaving other modules writable; change
the Makefile so either (A) chmod only the two affected modules (target the paths
used later: $(shell go env GOMODCACHE)/k8s.io/[email protected] and $(shell go env
GOMODCACHE)/k8s.io/[email protected]) before running $(TOOL_GO_TO_PROTOBUF),
or (B) after the cleanup steps (the rm -rf and go mod download lines) run a
chmod to remove write permission from the whole k8s.io tree to restore read-only
state; update the block around the existing chmod -R +w ... and the cleanup
lines so the chosen approach is applied.
- Around line 403-407: Make a tracked TODO in the Makefile next to the
go-to-protobuf upgrade: add a one-line TODO comment near the
$(TOOL_GO_TO_PROTOBUF) / USE_NIX block that references a follow-up issue number
to sync the Nix-pinned code-generator version with this upgrade (e.g., "TODO:
follow-up `#NNN` — update Nix pin for code-generator when bumping go-to-protobuf
to v0.33.1"); ensure it clearly mentions the target $(TOOL_GO_TO_PROTOBUF) and
the USE_NIX conditional so reviewers can find and act on it later.
In `@pkg/client/informers/externalversions/factory.go`:
- Around line 188-209: The doc example incorrectly references
factory.WaitForStop(); update the generated comment to use the actual method
name factory.Shutdown() (and keep surrounding example calls like
NewSharedInformerFactory, ForResource, Start, WaitForCacheSync intact) so the
snippet compiles; since this is a generated file, also file an upstream
informer-gen issue to correct the template that emits WaitForStop().
🧹 Nitpick comments (1)
Makefile (1)
483-484: Hardcoded@v0.33.1version appears in multiple places — consider extracting a variable.The version
v0.33.1for k8s.io modules is hardcoded on lines 406, 483, 484, 497, 498, and 779. If the k8s.io dependency version changes, all of these must be updated in lockstep.Suggested improvement
+# k8s.io code-generator / API module version — keep in sync with go.mod +K8S_CODEGEN_VERSION := v0.33.1 + $(TOOL_GO_TO_PROTOBUF): Makefile ifneq ($(USE_NIX), true) - go install k8s.io/code-generator/cmd/[email protected] + go install k8s.io/code-generator/cmd/go-to-protobuf@$(K8S_CODEGEN_VERSION) endifThen use
$(K8S_CODEGEN_VERSION)everywhere@v0.33.1appears in the codegen targets.
Comment on lines
+480
to
+498
| chmod -R +w $(shell go env GOMODCACHE)/k8s.io 2>/dev/null || true | ||
| mkdir -p $(GOPATH)/src/k8s.io | ||
| rm -rf $(GOPATH)/src/k8s.io/api $(GOPATH)/src/k8s.io/apimachinery | ||
| cp -r $(shell go env GOMODCACHE)/k8s.io/[email protected] $(GOPATH)/src/k8s.io/api | ||
| cp -r $(shell go env GOMODCACHE)/k8s.io/[email protected] $(GOPATH)/src/k8s.io/apimachinery | ||
| mkdir -p $(GOPATH)/src/github.com/argoproj/argo-workflows | ||
| rm -rf $(GOPATH)/src/github.com/argoproj/argo-workflows/v3 | ||
| ln -s $(CURDIR) $(GOPATH)/src/github.com/argoproj/argo-workflows/v3 | ||
| $(TOOL_GO_TO_PROTOBUF) \ | ||
| --go-header-file=./hack/custom-boilerplate.go.txt \ | ||
| --packages=github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1 \ | ||
| --apimachinery-packages=+k8s.io/apimachinery/pkg/util/intstr,+k8s.io/apimachinery/pkg/api/resource,k8s.io/apimachinery/pkg/runtime/schema,+k8s.io/apimachinery/pkg/runtime,k8s.io/apimachinery/pkg/apis/meta/v1,k8s.io/api/core/v1,k8s.io/api/policy/v1 \ | ||
| --proto-import $(GOPATH)/src | ||
| # Delete the link | ||
| [ -e ./v3 ] && rm -rf v3 | ||
| --proto-import $(GOPATH)/src \ | ||
| --output-dir $(GOPATH)/src | ||
| # Clean up and restore k8s.io modules that go-to-protobuf corrupted | ||
| rm -rf $(GOPATH)/src/k8s.io/api $(GOPATH)/src/k8s.io/apimachinery | ||
| rm -rf $(GOPATH)/src/github.com/argoproj/argo-workflows/v3 | ||
| rm -rf $(shell go env GOMODCACHE)/k8s.io/[email protected] $(shell go env GOMODCACHE)/k8s.io/[email protected] | ||
| go mod download k8s.io/[email protected] k8s.io/[email protected] |
Contributor
There was a problem hiding this comment.
chmod -R +w on all k8s.io modules leaves non-target modules writable after cleanup.
Line 480 makes the entire $(GOMODCACHE)/k8s.io directory tree writable, but lines 497–498 only delete and re-download [email protected] and [email protected]. Other k8s.io modules in the cache (e.g., client-go, kube-openapi, etc.) remain writable, which breaks Go's module cache contract (read-only = verified).
Consider either:
- Narrowing the
chmodto only the two modules that need it, or - Restoring read-only permissions on the entire
k8s.iotree after generation.
Option A: Narrow the chmod scope
- chmod -R +w $(shell go env GOMODCACHE)/k8s.io 2>/dev/null || true
+ chmod -R +w $(shell go env GOMODCACHE)/k8s.io/[email protected] 2>/dev/null || true
+ chmod -R +w $(shell go env GOMODCACHE)/k8s.io/[email protected] 2>/dev/null || trueOption B: Restore read-only after cleanup
go mod download k8s.io/[email protected] k8s.io/[email protected]
+ chmod -R a-w $(shell go env GOMODCACHE)/k8s.io 2>/dev/null || true
rm -rf vendor🤖 Prompt for AI Agents
In `@Makefile` around lines 480 - 498, The chmod -R +w on $(shell go env
GOMODCACHE)/k8s.io makes the entire k8s.io cache writable but only [email protected]
and [email protected] are re-downloaded, leaving other modules writable;
change the Makefile so either (A) chmod only the two affected modules (target
the paths used later: $(shell go env GOMODCACHE)/k8s.io/[email protected] and $(shell
go env GOMODCACHE)/k8s.io/[email protected]) before running
$(TOOL_GO_TO_PROTOBUF), or (B) after the cleanup steps (the rm -rf and go mod
download lines) run a chmod to remove write permission from the whole k8s.io
tree to restore read-only state; update the block around the existing chmod -R
+w ... and the cleanup lines so the chosen approach is applied.
Comment on lines
+188
to
+209
| // | ||
| // It is typically used like this: | ||
| // | ||
| // ctx, cancel := context.Background() | ||
| // defer cancel() | ||
| // factory := NewSharedInformerFactory(client, resyncPeriod) | ||
| // defer factory.WaitForStop() // Returns immediately if nothing was started. | ||
| // genericInformer := factory.ForResource(resource) | ||
| // typedInformer := factory.SomeAPIGroup().V1().SomeType() | ||
| // factory.Start(ctx.Done()) // Start processing these informers. | ||
| // synced := factory.WaitForCacheSync(ctx.Done()) | ||
| // for v, ok := range synced { | ||
| // if !ok { | ||
| // fmt.Fprintf(os.Stderr, "caches failed to sync: %v", v) | ||
| // return | ||
| // } | ||
| // } | ||
| // | ||
| // // Creating informers can also be created after Start, but then | ||
| // // Start must be called again: | ||
| // anotherGenericInformer := factory.ForResource(resource) | ||
| // factory.Start(ctx.Done()) |
Contributor
There was a problem hiding this comment.
Minor: generated doc comment references non-existent WaitForStop() method.
Line 194 references factory.WaitForStop() but the actual interface method is Shutdown() (Line 228). This is a documentation bug in the generated code from informer-gen. If someone copies this example, they'll get a compile error.
Since this file is generated (DO NOT EDIT), this can't be fixed here — it would need to be reported upstream to the k8s code-generator.
🤖 Prompt for AI Agents
In `@pkg/client/informers/externalversions/factory.go` around lines 188 - 209, The
doc example incorrectly references factory.WaitForStop(); update the generated
comment to use the actual method name factory.Shutdown() (and keep surrounding
example calls like NewSharedInformerFactory, ForResource, Start,
WaitForCacheSync intact) so the snippet compiles; since this is a generated
file, also file an upstream informer-gen issue to correct the template that
emits WaitForStop().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13620
Motivation
The k8s.io code-generator tool needs to be upgraded to v0.33.1 to resolve compatibility issues with newer Kubernetes API versions and to fix issues with the module cache handling during code generation.
Modifications
go-to-protobuffrom v0.21.5 to v0.33.1pkg/apis/workflow/v1alpha1/generated.prototarget with:pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.gotarget to:kube_codegen.shfrom v0.33.1 instead of deprecatedgenerate-groups.shYes, it's a pretty horrific change.
It deliberately avoids updating nix at this stage, as @isubasinghe was working on a separate patch for that.
Verification
Documentation
No documentation changes needed - this is an internal tooling upgrade.
Summary by CodeRabbit
Release Notes