chore: updated go-to-protobuf #15545
Conversation
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
isubasinghe
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe PR implements a new proto dependency management system using Buf. It introduces argo-proto.yaml for declaring proto dependencies, a proto-export tool to manage and export those dependencies, updates Makefile to support proto_vendor targeting, and rewires protocol buffer type references to use absolute package paths for Kubernetes API compatibility. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 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 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@hack/proto-export/main.go`:
- Around line 47-107: The function bundleDependencies has mixed tabs/spaces and
inconsistent indentation making it hard to read; run gofmt (or an equivalent
formatter) and normalize indentation so each nesting level uses a single tab (or
gofmt default) across the function, including blocks that create
vendorDir/moduleDir, write buf.yaml (bufConfigFile), iterate dep.Bundle and the
git steps slice (steps) and loop; ensure the for _, args := range steps loop and
the nested exec.Command handling match the surrounding indentation and remove
any stray extra spaces/tabs so the whole function compiles with gofmt
formatting.
- Around line 109-223: The exportDependencies function has excessive blank lines
and inconsistent indentation making control flow hard to read; run gofmt and
remove unnecessary blank lines and align blocks (particularly around the if
dep.Buf != nil / else if len(dep.Bundle) > 0 branches, the target/source
assignments, the os.MkdirAll error handling, and the cmd := exec.Command(...) +
cmd.Stdout/Stderr blocks) so indentation is consistent and each logical
statement group is compact; ensure braces and nested blocks for
exportDependencies, dep.Buf, dep.Bundle, os.MkdirAll(target,...), and cmd.Run()
are properly indented and no superfluous empty lines remain.
In `@Makefile`:
- Around line 185-190: The proto_vendor Makefile target (proto_vendor) runs "go
run hack/proto-export/*.go" without forcing module mode; prefix that invocation
with GOFLAGS=-mod=mod (same pattern used elsewhere) so the command becomes
GOFLAGS=-mod=mod go run hack/proto-export/*.go to avoid picking up a vendor-mode
GOPATH/vendor or stale vendor dir and ensure dependencies like gopkg.in/yaml.v3
resolve correctly; update the proto_vendor target where TOOL_BUF and the touch
proto_vendor line appear.
🧹 Nitpick comments (8)
.gitignore (1)
57-59: Consider adding a brief comment above these entries for future maintainers.The relationship between these patterns and the
hack/proto-exporttool isn't obvious at a glance:
.*-vendorcatches bundle vendor dirs like.k8s.io-vendor.github.comcatches the nested.github.com/…-vendortrees created when dependency keys containgithub.com/…proto_vendoris the export output directoryThis is fine functionally, but note that if a new dependency is added with a different domain prefix (e.g.,
gitlab.com/…), a new gitignore entry would be needed.hack/proto-export/vendor.go (1)
1-1: Empty file — remove or populate.This file contains only
package mainwith no declarations, imports, or build tags. Bothmain.goandconfig.goalready declare the same package, so this file is redundant. If vendor-related logic was planned, consider adding a TODO; otherwise, remove it to avoid confusion.argo-proto.yaml (2)
22-22: Consider adding a comment for the opaque buf ref.The googleapis ref
004180b77378443887d3b55cabc00384is a BSR commit hash, which is harder to audit or update compared to semantic version tags. A comment documenting what this corresponds to (e.g., a date or upstream version) would help future maintainers.
25-25: The empty-string bundle key""is functional but non-obvious.This causes the repo to be cloned directly into the module directory itself (since
filepath.Join(moduleDir, "")returnsmoduleDir). A brief inline comment explaining this convention would improve readability.hack/proto-export/main.go (3)
89-103: Consider cleaning up.gitdirectories after checkout.The shallow-clone sequence leaves a
.gitdirectory inside each bundle target. While these directories are gitignored, they consume unnecessary disk space and could confuse tools that walk the directory tree. Addingrm -rf .gitas a final step (oros.RemoveAll(filepath.Join(targetDir, ".git"))) after checkout would keep the vendor directories lean.Proposed fix
for _, args := range steps { cmd := exec.Command(args[0], args[1:]...) cmd.Dir = targetDir if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("git command %v failed in %s: %s: %w", args, targetDir, string(output), err) } } + // Remove .git directory to save space in vendored output + if err := os.RemoveAll(filepath.Join(targetDir, ".git")); err != nil { + return fmt.Errorf("failed to remove .git in %s: %w", targetDir, err) + } }
131-141: Simplify the sub-path detection logic.The dual-branch separator check is unnecessarily complex. Since the YAML config always uses forward slashes and
filepath.Baseworks correctly on forward-slash paths even on Windows, this can be simplified:Proposed simplification
- if filepath.Separator == '/' && (filepath.Base(key) != key) { - target = filepath.Join(outDir, key) - } else if filepath.Separator != '/' && (filepath.ToSlash(key) != key) { - // Handle windows if necessary, though key is usually forward-slash - target = filepath.Join(outDir, key) - } + if strings.Contains(key, "/") { + target = filepath.Join(outDir, key) + }This would require adding
"strings"to the imports.
18-18: Hardcoded config path"argo-proto.yaml"assumes the tool is always run from the repo root.This is fine given the Makefile always invokes it from the root (
go run hack/proto-export/*.go), but consider accepting the config path as a flag for flexibility, or at minimum add a comment documenting this assumption.Makefile (1)
241-258: Symlink creation should be idempotent — verifygithub.com/argoprojdoesn't collide with repo content.Lines 245–247 create
github.com/argoproj/argo-workflowssymlink and av3symlink. Line 257 cleans them up. This is fine for theprotocdefine, but note that the same pattern is also used in thegenerated.prototarget (Lines 478–480) and theopenapi_generated.gotarget (Line 756). If these targets run concurrently (e.g., viamake -j), the shared symlinks could cause races.Given that these targets have sequential dependencies in practice, this is likely safe — but worth being aware of.
Signed-off-by: isubasinghe <isitha@pipekit.io>
1 participant
Fixes #13620
Motivation
The k8s.io code gen tool needs to be updated.
Modifications
-mod=modbut unfortunatelygo-to-protobufis still reliant upon mutating the vendor folder, so we also perform some resetting of the vendor directory.Verification
All make targets work as expected.
Documentation
Summary by CodeRabbit