Introduce an initial set of DevEx features#10
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const maxFileSize = 1024 * 1024 * 1024 | ||
|
|
||
| for { | ||
| header, err := tarReader.Next() |
The central concept in our developer experience is the Project. Introduce the Project API in preparation for adding the tooling. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughAdds a Project API, projectfile parsing, dependency/schema management, multi-language schema generators, function builders, packaging and push flows, a local dev control plane, CLI subcommands (project, dependency, function, composition, xrd, render), and numerous supporting utilities and tests. ChangesProject developer tooling and CLI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related issues
Thank you — if you want I can (1) break the large layers into finer review checkpoints for focused reviewer passes, or (2) produce targeted review checklists for any specific layer (e.g., dependency manager, function builders, or CLI changes). Which should I do next? |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
internal/project/certs/tls.go-153-160 (1)
153-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuick sanity check on the completeness condition — should this be
&&instead of||?Comparing with the analogous check in
loadOrGenerateCAat Line 103 (if len(kd) != 0 && len(cd) != 0), the server-cert check here uses||. As written, the function will treat the secret as already complete whenever any one oftls.crt,tls.key, orca.crtis non-empty — which seems at odds with the log message just below ("Server certificates are empty or not complete, generating a new pair...") and would cause us to skip regeneration when the secret is only partially populated (e.g., someone hand-setca.crt).Was the
||intentional here, or should all three fields be required for "complete"? If the latter:🐛 Proposed fix
- if len(sec.Data[corev1.TLSCertKey]) != 0 || len(sec.Data[corev1.TLSPrivateKeyKey]) != 0 || len(sec.Data[SecretKeyCACert]) != 0 { + if len(sec.Data[corev1.TLSCertKey]) != 0 && len(sec.Data[corev1.TLSPrivateKeyKey]) != 0 && len(sec.Data[SecretKeyCACert]) != 0 { e.log.Info("TLS secret contains server certificate.", "secret", nn.Name) return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/certs/tls.go` around lines 153 - 160, The completeness check for the TLS secret currently returns early if any one of sec.Data[corev1.TLSCertKey], sec.Data[corev1.TLSPrivateKeyKey], or sec.Data[SecretKeyCACert] is non-empty; change this to require all three to be present (use logical AND semantics) so the function only treats the secret as complete when tls.crt AND tls.key AND ca.crt are non-empty (mirror the behavior in loadOrGenerateCA) — update the condition that sets create=false and the early return accordingly in the code around the TLS secret check.internal/docker/docker.go-508-525 (1)
508-525: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winWrap the new filesystem/read errors with user context.
Thanks for adding these helpers. A few paths here still return raw errors (
MkdirAll,Create,io.Copy,io.ReadAll), so users won't see which container path or extracted entry failed. Could we wrap those with the source path / entry name the same way the Docker calls are wrapped?Suggested fix
case tar.TypeDir: if err := fs.MkdirAll(cleanedPath, 0o755); err != nil { - return err + return errors.Wrapf(err, "cannot create directory %q while copying %q from container %q", cleanedPath, path, cid) } case tar.TypeReg: outFile, err := fs.Create(cleanedPath) if err != nil { - return err + return errors.Wrapf(err, "cannot create file %q while copying %q from container %q", cleanedPath, path, cid) } @@ - if _, err := io.Copy(outFile, limitedReader); err != nil { + if _, err := io.Copy(outFile, limitedReader); err != nil { if cerr := outFile.Close(); cerr != nil { err = errors.Wrap(cerr, "error while closing file") } - return err + return errors.Wrapf(err, "cannot write extracted file %q from container path %q", cleanedPath, path) } @@ - return io.ReadAll(reader) + b, err := io.ReadAll(reader) + if err != nil { + return nil, errors.Wrapf(err, "failed to read tarball copied from container path %q", path) + } + return b, nilAs per coding guidelines, "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."
Also applies to: 541-547
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/docker.go` around lines 508 - 525, Wrap all filesystem and reader errors with user-facing context using crossplane-runtime/pkg/errors the same way Docker call errors are wrapped: when calling fs.MkdirAll(cleanedPath), fs.Create(cleanedPath), io.Copy(outFile, limitedReader), and any io.ReadAll usage (lines around the io.ReadAll block), wrap returned errors with errors.Wrapf or errors.Wrap including the tar header name (header.Name) and the container/source path being extracted, and preserve the existing close-error wrapping for outFile.Close; reference the symbols fs.MkdirAll, fs.Create, tarReader, limitedReader, io.Copy, io.ReadAll, outFile.Close, header.Name and maxFileSize so the new error messages show which archive entry/path failed and include actionable context.internal/docker/docker.go-492-523 (1)
492-523:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on oversized tar entries to prevent silent corruption.
The current code uses
io.LimitReaderto cap bytes read from the tar stream, but this causes files larger than 1GiB to be silently truncated. Sinceio.Copystops at EOF (not an error), the function returnsnileven though the extracted file is incomplete and corrupted. This could lead to downstream packaging using broken artifacts without knowing.Adding an upfront check on
header.Sizeand rejecting oversized entries is the right approach:Suggested fix
// Limit files to 1GiB to avoid excessive memory usage. const maxFileSize = 1024 * 1024 * 1024 @@ case tar.TypeReg: + if header.Size > maxFileSize { + return errors.Errorf("cannot copy %q from container path %q: file exceeds 1GiB limit", header.Name, path) + } outFile, err := fs.Create(cleanedPath) if err != nil { return err } - - limitedReader := io.LimitReader(tarReader, maxFileSize) - if _, err := io.Copy(outFile, limitedReader); err != nil { + if _, err := io.Copy(outFile, tarReader); err != nil { if cerr := outFile.Close(); cerr != nil { err = errors.Wrap(cerr, "error while closing file") } return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/docker.go` around lines 492 - 523, The tar extraction silently truncates large files; update the loop handling tar.TypeReg in the function reading from tarReader to first check header.Size against maxFileSize and return a clear error if header.Size > maxFileSize (include header.Name and header.Size in the message), before creating outFile or using io.LimitReader; keep the existing use of maxFileSize, header, cleanedPath, outFile and tarReader but fail fast on oversized entries to avoid producing corrupted files.internal/docker/docker.go-507-525 (1)
507-525:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve file and directory modes when extracting from container tarballs.
Files copied from containers currently lose their permission bits. When
CopyFromContainerextracts a tar archive, directories are hardcoded to0o755and regular files get default mode viafs.Create(), which discards theheader.Modefrom the tar entry. This breaks executable binaries or scripts that need specific permissions to function correctly.The tar header provides the original file mode—use
os.FileMode(header.Mode).Perm()to extract the permission bits and apply them when creating files and directories withfs.OpenFile()andfs.MkdirAll().Suggested fix
+ "os" @@ switch header.Typeflag { case tar.TypeDir: - if err := fs.MkdirAll(cleanedPath, 0o755); err != nil { + if err := fs.MkdirAll(cleanedPath, os.FileMode(header.Mode).Perm()); err != nil { return err } case tar.TypeReg: - outFile, err := fs.Create(cleanedPath) + if err := fs.MkdirAll(filepath.Dir(cleanedPath), 0o755); err != nil { + return err + } + outFile, err := fs.OpenFile(cleanedPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode).Perm()) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/docker.go` around lines 507 - 525, The extraction code currently hardcodes directory mode (0o755) and uses fs.Create for files, losing permission bits from the tar header; update the tar.TypeDir and tar.TypeReg branches to preserve header.Mode by converting header.Mode to os.FileMode(header.Mode).Perm() and passing that mode into fs.MkdirAll and fs.OpenFile (use os.O_CREATE|os.O_WRONLY|os.O_TRUNC) instead of fs.Create; after writing, ensure the file is closed and if the filesystem abstraction doesn't accept modes on create, call fs.Chmod(cleanedPath, os.FileMode(header.Mode).Perm()) to set the correct permissions (referencing cleanedPath, header.Mode, tar.TypeDir, tar.TypeReg, maxFileSize, fs.MkdirAll, fs.OpenFile, fs.Create).cmd/crossplane/project/stop.go-47-50 (1)
47-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap errors with command context and preserve the parse cause.
Thanks for adding this command — one blocker: Line 49 and Line 66 return raw errors, and Line 57 masks all parse failures as “not a project directory”. That makes troubleshooting harder for users (e.g., malformed YAML vs missing file).
As per coding guidelines, "`**/*.go`: Use crossplane-runtime/pkg/errors for wrapping... Ensure all error messages are meaningful to end users, include context, and suggest next steps."Suggested fix
projFilePath, err := filepath.Abs(c.ProjectFile) if err != nil { - return err + return errors.Wrapf(err, "cannot resolve project definition path %q", c.ProjectFile) } @@ prj, err := projectfile.Parse(projFS, projFileName) if err != nil { - return errors.New("this is not a project directory; use --control-plane-name to specify the control plane name") + return errors.Wrapf(err, "cannot read project definition %q; run this command from a project directory or use --control-plane-name", projFilePath) } @@ if err := sp.WrapWithSuccessSpinner("Tearing down control plane", func() error { return controlplane.TeardownLocalDevControlPlane(ctx, name, c.RegistryDir) }); err != nil { - return err + return errors.Wrap(err, "cannot tear down local dev control plane") }Also applies to: 55-58, 63-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/stop.go` around lines 47 - 50, Wrap and annotate raw errors from filepath.Abs(c.ProjectFile) and the project parsing logic using crossplane-runtime/pkg/errors so the original causes are preserved and users get actionable context; e.g., replace returns like "return err" with errors.Wrapf(err, "failed to resolve absolute path for project file %q", c.ProjectFile) for the filepath.Abs call, and for the project parse/validate code detect os.IsNotExist to return a clear "project file not found" message but otherwise return errors.Wrapf(parseErr, "failed to parse project file %q", c.ProjectFile) so malformed YAML and other parse failures are preserved and surfaced rather than being masked as "not a project directory".internal/project/functions/go.go-55-128 (1)
55-128:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffGlobal logger modification could affect concurrent builds.
Line 58 modifies the global
logpackage's output withlog.SetOutput(io.Discard). If multiple goroutines are building Go functions concurrently, this could cause race conditions or unexpected log suppression in other parts of the system that use the standard logger.Consider whether:
- This builder is guaranteed to run in isolation (no concurrent builds)
- If concurrent builds are possible, whether ko provides a way to configure logging per-builder instance, or if you should protect this with a mutex
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/functions/go.go` around lines 55 - 128, The code currently silences the global logger with log.SetOutput(io.Discard) inside goBuilder.Build which can race with other concurrent builds; instead avoid mutating the global logger. Fix by either (A) using a per-builder logger option if available when calling build.NewGo (check for any build.WithLogger / WithLog option) or (B) if no per-builder option exists, wrap the global suppression with a mutex and restore the previous writer: capture old := log.Writer(), lock a package-level mutex, call log.SetOutput(io.Discard) before invoking builder (build.NewGo / builder.Build), defer restoring with log.SetOutput(old) and unlock in the defer, or simply remove the log.SetOutput call if builds run in shared context. Ensure changes target the log.SetOutput usage in goBuilder.Build and the build.NewGo/builder.Build call sites.internal/project/functions/python.go-104-107 (1)
104-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBuild the Python venv per target architecture.
Thanks for adding the Python builder. The current flow builds
/fnonce, before the architecture loop, then reuses that same tarball for every output image. That means anarm64image can still contain the host/build-container’samd64venv, entrypoints, and any native wheels, so multi-arch outputs will fail at runtime. Could we make the build step architecture-aware and run it inside each per-arch branch instead of reusing onevenvTar?Also applies to: 123-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/functions/python.go` around lines 104 - 107, The venv is currently built once (venvTar := b.buildVenv(ctx, c)) before the architecture loop and reused for all arches; change the flow so buildVenv is invoked inside the per-architecture branch instead of once globally: move the call to b.buildVenv(ctx, c) into the loop that handles each target architecture (the same place where you prepare per-arch image outputs) so each architecture gets its own venvTar built in that architecture context; update any variables (venvTar, err) scoping accordingly and ensure the per-arch error handling mirrors the original pattern.cmd/crossplane/dependency/add.go-116-131 (1)
116-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
--git-pathunless--git-refis set.Right now
--git-pathalone is silently ignored because the code falls through to xpkg parsing. This should return a clear validation error so users don’t add the wrong dependency type by accident.As per coding guidelines, "
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/dependency/add.go` around lines 116 - 131, Add explicit validation to reject a provided GitPath when GitRef is not set: before falling through to xpkg parsing (near the block that checks c.GitRef and constructs v1alpha1.Dependency with v1alpha1.GitDependency), add a check like if c.GitPath != "" && c.GitRef == "" then return an error (e.g. "git-path requires git-ref to be set") so --git-path alone no longer gets silently ignored; update any error messages to clearly instruct the user to set --git-ref when using --git-path.internal/project/render.go-49-55 (1)
49-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter to function dependencies before creating
pkgv1.Functionobjects.Current logic includes every non-API-only xpkg dependency. If a project has provider/configuration xpkgs, this generates invalid Function manifests. Please gate on dependency metadata (kind/apiVersion) so only function packages are converted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/render.go` around lines 49 - 55, The loop currently converts every non-API-only xpkg into pkgv1.Functions; change the guard in the proj.Spec.Dependencies iteration (the block checking dep.Type == DependencyTypeXpkg and dep.Xpkg != nil && !dep.Xpkg.APIOnly) to additionally verify the package metadata identifies a function package (e.g., check dep.Xpkg.Kind and dep.Xpkg.APIVersion or equivalent fields) and only proceed to create pkgv1.Function objects when those metadata values match the expected function kind/apiVersion; update the conditional that leads to Function construction so provider/configuration xpkgs are skipped.apis/dev/v1alpha1/validate.go-121-123 (1)
121-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that
typematches the populated source field.
sourceCount == 1is necessary but not sufficient. A dependency can currently declaretype: xpkgwhile onlygitis set, and still pass validation. Please enforce exact type-to-source consistency to prevent invalid API objects.As per coding guidelines, "
**/apis/**: Focus on API design following Kubernetes API conventions... Ensure error messages in validation are user-friendly. Pay attention to API consistency."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/dev/v1alpha1/validate.go` around lines 121 - 123, sourceCount only ensures one source is set but doesn't verify the declared Type matches that populated source; after the existing sourceCount check, switch on the dependency's Type (field Type) and validate the corresponding source field (Xpkg, Git, Http, K8s) is the one populated (e.g., if Type == "xpkg" then Xpkg must be non-nil/non-empty and Git/Http/K8s must be empty), appending user-friendly errors to errs via errors.New when mismatches occur; also handle unknown Type values with a clear error message so validation fails for inconsistent or invalid type-to-source combinations.cmd/crossplane/project/run.go-302-306 (1)
302-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow all kubeconfig load errors.
Line 302 currently treats any
LoadFromFileerror as “file missing” and starts fresh, which can overwrite a real kubeconfig after permission/parse failures. Please only fall back onos.IsNotExist(err)and return other errors with context.Suggested fix
+import "os" ... existing, err := clientcmd.LoadFromFile(defaultPath) if err != nil { - // If the file doesn't exist, start fresh. - existing = clientcmdapi.NewConfig() + if os.IsNotExist(err) { + existing = clientcmdapi.NewConfig() + } else { + return errors.Wrapf(err, "cannot read kubeconfig at %q", defaultPath) + } }As per coding guidelines, "
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages... CLI error messages must be especially user-friendly - avoid internal error details, provide actionable guidance."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/run.go` around lines 302 - 306, The code currently treats any error from clientcmd.LoadFromFile(defaultPath) as "file missing" and silently creates a new config; instead check the error with os.IsNotExist(err) and only then set existing = clientcmdapi.NewConfig(); for all other errors return a wrapped/contextual error (e.g., "failed to load kubeconfig at defaultPath: <err>") so permission or parse failures are propagated. Locate the call to clientcmd.LoadFromFile and update the error branch to use os.IsNotExist(err) and return the error otherwise, leaving the existing variable and clientcmdapi.NewConfig() logic intact.cmd/crossplane/dependency/add.go-156-165 (1)
156-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
name.NewTag()to properly parse xpkg references with registry ports.The current
strings.Cut(c.Package, ":")approach breaks valid references likeexample.com:5000/repo:v1.2.3by splitting on the first colon instead of the last one. Sincego-containerregistry/pkg/nameis already imported and used throughout the codebase, consider tryingname.NewTag()before falling back tostrings.Cut(). This follows the pattern already established ininternal/xpkg/resolver.go.As a secondary concern,
--git-pathis silently ignored when--git-refis not provided. Per the CLI guidelines, adding a validation error would improve user experience: "git-path is only valid with git-ref".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/dependency/add.go` around lines 156 - 165, Replace the fragile strings.Cut(c.Package, ":") parsing with a try-first name.NewTag(c.Package) parse (as used in internal/xpkg/resolver.go) to correctly handle registry host:port forms; if name.NewTag succeeds, extract the repository part into pkg and the tag into version, otherwise fall back to the existing strings.Cut behavior. Also add CLI validation in the add command to error when git-path is provided without git-ref (e.g., in the command handling that reads c.GitPath / c.GitRef) and return a clear validation error "git-path is only valid with git-ref".internal/project/push.go-55-58 (1)
55-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against zero concurrency to avoid deadlock
Could we clamp concurrency to at least 1? Right now, if
n == 0, Line 120 creates a zero-capacity semaphore and every worker blocks on Line 123.Suggested fix
func PushWithMaxConcurrency(n uint) PusherOption { return func(p *realPusher) { - p.maxConcurrency = n + if n == 0 { + p.maxConcurrency = 1 + return + } + p.maxConcurrency = n } }Also applies to: 120-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/push.go` around lines 55 - 58, Clamp concurrency to at least 1: in PushWithMaxConcurrency set p.maxConcurrency = 1 when n == 0 (e.g., p.maxConcurrency = uint(math.Max(1, int(n))) or a simple if n == 0 { p.maxConcurrency = 1 } else { p.maxConcurrency = n }). Also ensure the semaphore creation that reads p.maxConcurrency (the code that builds the semaphore/capacity used by the workers in realPusher) uses max(1, p.maxConcurrency) so you never create a zero-capacity semaphore; update both PushWithMaxConcurrency and the semaphore-instantiation site accordingly.internal/dependency/manager.go-129-137 (1)
129-137:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPrevent nil-pointer panics by enforcing manager invariants
Could we make
NewManagerguarantee non-nilproj.Spec.Paths,resolver, andclient(or fail fast with a clear error)? As written, Line 132 / Line 166 / Line 186 can panic if any of these are unset.As per coding guidelines, "**/*.go: ... Ensure all error messages are meaningful to end users ... include context about what the user was trying to do, and suggest next steps when possible."
Also applies to: 165-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dependency/manager.go` around lines 129 - 137, NewManager must validate required inputs and fail fast with clear errors: change NewManager signature to return (*Manager, error), then at the start validate proj != nil and that proj.Spec.Paths is non-nil (and that proj.Spec.Paths.Schemas is set) before creating schemaFS; validate that provided ManagerOption values produce non-nil resolver and client (or set safe defaults) before using them; if any invariant is missing, return a descriptive error (e.g., "missing project paths: set Project.Spec.Paths.Schemas" or "missing resolver/client: provide via ManagerOption") rather than letting code dereference nils; update managerOptions handling to surface these errors when constructing Manager (symbols: NewManager, managerOptions, proj.Spec.Paths, schemaFS, resolver, client).internal/schemas/generator/kcl.go-230-243 (1)
230-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose opened files during KCL transform walk
Thanks for this transform flow. Could we close both
srcFileanddestFileafter copy? Right now Lines 230 and 235 open handles that are never closed, which can exhaust descriptors on larger inputs.Suggested fix
srcFile, err := fs.Open(path) if err != nil { return errors.Wrapf(err, "failed to open source file %s", path) } +defer func() { _ = srcFile.Close() }() destFile, err := fs.Create(newFilePath) if err != nil { return errors.Wrapf(err, "failed to create destination file %s", newFilePath) } +defer func() { _ = destFile.Close() }() _, err = io.Copy(destFile, srcFile)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/kcl.go` around lines 230 - 243, The srcFile and destFile opened in the KCL transform (calls to fs.Open and fs.Create in generator/kcl.go around the io.Copy) are not closed and can leak file descriptors; after successfully opening each file, ensure you close them (use defer srcFile.Close() and defer destFile.Close() immediately after their nil-error checks) and optionally check/return any Close errors (wrap with errors.Wrapf) after io.Copy to surface close failures.internal/schemas/generator/go.go-999-1000 (1)
999-1000: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse crossplane-runtime error wrapping consistently
Lines 999-1000 and 1023-1025 use
fmt.Errorfwhile the rest of the file useserrors.Wrap/errors.Wrapf. Can we align these with the rest of the codebase for consistency? The error messages could also be more descriptive—consider matching the pattern used elsewhere in the file (e.g., "failed to parse Go code" on line 1035) to provide clearer context to users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 999 - 1000, Replace the fmt.Errorf usages in go.go (the return "", fmt.Errorf("parsing failed: %w", err) call and the other fmt.Errorf blocks around lines ~1023-1025) with crossplane-runtime style errors.Wrap/errors.Wrapf to match the rest of the file; use a clearer message consistent with existing patterns (e.g., errors.Wrap(err, "failed to parse Go code") or errors.Wrapf(err, "failed to parse Go code: %s", someContext)) so the wrapped error preserves the original error while providing the same descriptive context used elsewhere.internal/schemas/generator/python.go-290-305 (1)
290-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate destination directory before copying
v1.pyThanks for the transform logic here. Could we add
MkdirAll(destDir, ...)before Line 303? Without it, writingtargetDir/.../v1.pycan fail when the directory hasn’t been created yet.Suggested fix
destDir := filepath.Join(targetDir, "io", "k8s", "apimachinery", "pkg", "apis", "meta") destPath := filepath.Join(destDir, "v1.py") +if err := fs.MkdirAll(destDir, os.ModePerm); err != nil { + return errors.Wrapf(err, "failed to create directory %s", destDir) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/python.go` around lines 290 - 305, The code copies v1.py into destPath but doesn't ensure the destination directory exists; before writing (prior to the afero.WriteFile call) call MkdirAll on destDir using the provided afero FS and appropriate permissions (e.g., 0755) to create the directory hierarchy; locate the block handling destDir/destPath in the function containing the ReadFile/Stat/WriteFile sequence and insert an afero.MkdirAll(fs, destDir, 0755) (or equivalent) and handle/return any error from that call.internal/project/build.go-504-515 (1)
504-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScan every YAML document before dropping the file.
yaml.UnmarshalintoTypeMetaonly inspects the first document in a multi-doc manifest. That means a file whose later document is an XRD, Composition, or Operation gets skipped entirely, which will silently omit valid resources from the built package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/build.go` around lines 504 - 515, The current logic uses yaml.Unmarshal into metav1.TypeMeta (u) which only checks the first YAML document, causing multi-doc manifests to be skipped; change the check to iterate over all YAML documents in the file (e.g., use yaml.NewDecoder(bytes.NewReader(bs)) or a YAML decoder that supports multiple docs) and for each decoded metav1.TypeMeta call u.GroupVersionKind().String() and test slices.Contains(gvks, ...) so the file is only dropped if none of the documents match; update the code around bs, yaml.Unmarshal, metav1.TypeMeta (u), and the slices.Contains(gvks, u.GroupVersionKind().String()) check accordingly.internal/project/controlplane/controlplane.go-504-512 (1)
504-512:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlease reapply and wait for registry trust configuration before returning.
On the fast path, this returns as soon as an existing registry container is restarted, so a newly created/reused kind cluster never gets
/etc/containerd/certs.dpopulated. On the slow path,configureContainerdLocalRegistryreturns right after creating a fixed-name Job, so the control plane can be reported ready before trust is installed, and later runs won't rerun once that Job already exists. Could we make this step idempotent and wait for Job completion before reporting readiness?Also applies to: 554-556, 637-641
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/controlplane/controlplane.go` around lines 504 - 512, The fast-path early return after restarting an existing registry container (when docker.StartContainerByID returns successfully) must not return immediately — call configureContainerdLocalRegistry with the same context/inputs and wait for the registry-trust Job to finish before returning; make configureContainerdLocalRegistry idempotent (create-or-get) and add a deterministic wait/timeout path that polls the Job status (or uses a new waitForJobCompletion helper) so repeated runs do nothing if trust is already installed. Apply the same change to the slow-path return site (where the fixed-name Job is created) so that instead of returning right after Job creation the code waits for Job completion (using the idempotent create-or-wait behavior) before reporting readiness; reference functions/variables: docker.StartContainerByID, configureContainerdLocalRegistry, certDir, certSecret, and the fixed-name Job handling to locate the changes.internal/project/build.go-89-92 (1)
89-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp
maxConcurrencyaway from zero.If a caller sets this to
0, the semaphore inbuildFunctionsbecomes unbuffered and every worker blocks forever on the first send at Line 333. Could we treat0as1or reject it explicitly here?💡 Minimal fix
func BuildWithMaxConcurrency(n uint) BuilderOption { return func(b *realBuilder) { + if n == 0 { + n = 1 + } b.maxConcurrency = n } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/build.go` around lines 89 - 92, BuildWithMaxConcurrency currently assigns b.maxConcurrency directly, allowing 0 which makes the semaphore in buildFunctions unbuffered and deadlock; change BuildWithMaxConcurrency to clamp zero to one by setting b.maxConcurrency = 1 when n == 0 (i.e., replace the direct assignment with a conditional that sets 1 for n==0, otherwise n) so realBuilder.maxConcurrency is never zero and the semaphore remains buffered. Ensure you update any related comments/tests that assume zero is allowed.internal/project/controlplane/controlplane.go-520-530 (1)
520-530: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPlease preserve the underlying filesystem error here.
These
errors.New(...)returns drop whether setup failed because of permissions, a bad mount, disk space, etc. Wrapping the originalerrwith user-facing context would make local registry failures much easier to debug.💡 Example
if err := os.MkdirAll(certDir, 0o755); err != nil { //nolint:gosec // Container needs to read the dir. - return "", errors.New("failed to create cert directory") + return "", errors.Wrap(err, "failed to prepare TLS files for the local registry") } if err := os.WriteFile(filepath.Join(certDir, "ca.crt"), certSecret.Data[certs.SecretKeyCACert], 0o644); err != nil { //nolint:gosec // Container needs to read the file. - return "", errors.New("failed to write ca cert") + return "", errors.Wrap(err, "failed to write the local registry CA certificate") } if err := os.WriteFile(filepath.Join(certDir, "tls.crt"), certSecret.Data[corev1.TLSCertKey], 0o644); err != nil { //nolint:gosec // Container needs to read the file. - return "", errors.New("failed to write tls cert") + return "", errors.Wrap(err, "failed to write the local registry TLS certificate") } if err := os.WriteFile(filepath.Join(certDir, "tls.key"), certSecret.Data[corev1.TLSPrivateKeyKey], 0o644); err != nil { //nolint:gosec // Container needs to read the file. - return "", errors.New("failed to write tls key") + return "", errors.Wrap(err, "failed to write the local registry TLS private key") }As per coding guidelines, "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/controlplane/controlplane.go` around lines 520 - 530, The current writes in controlplane.go drop the underlying filesystem error; update the error returns in the os.MkdirAll and os.WriteFile calls that use certDir and certSecret (keys certs.SecretKeyCACert, corev1.TLSCertKey, corev1.TLSPrivateKeyKey) to wrap the original err using crossplane-runtime/pkg/errors (e.g., errors.Wrapf or errors.Wrap) and include contextual details like the target path (filepath.Join(certDir, "...")) and a brief user-facing hint (e.g., check permissions/mount/disk space) so the returned error preserves the original error and gives meaningful next steps.
🟡 Minor comments (9)
internal/project/certs/cert_generator.go-61-63 (1)
61-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDoc comment doesn't quite match what
Generateactually does — could we tighten it up?The comment says this "creates TLS Secret with 10 years expiration date that is valid for the given domains", but the function:
- returns raw PEM bytes (no Kubernetes Secret involved),
- doesn't set the expiration (
NotAfteris whatever the caller put oncert),- doesn't set DNS names (also driven by the caller's
cert).It looks like the doc was likely copy/pasted from a higher-level caller. Mind updating it to describe what this method really does so future readers don't get misled? Thanks!
📝 Suggested doc tweak
-// Generate creates TLS Secret with 10 years expiration date that is valid -// for the given domains. +// Generate produces a PEM-encoded RSA private key and an X.509 certificate +// built from the given template. If signer is nil, the certificate is +// self-signed using the generated key. func (*CertGenerator) Generate(cert *x509.Certificate, signer *CertificateSigner) (key []byte, crt []byte, err error) {As per coding guidelines: "Ensure all error messages are meaningful to end users… include context about what the user was trying to do" — the same spirit applies to doc comments documenting exported APIs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/certs/cert_generator.go` around lines 61 - 63, Update the doc comment for CertGenerator.Generate to accurately describe its behavior: state that Generate takes an x509.Certificate and a *CertificateSigner and returns PEM-encoded private key and certificate bytes (key, crt) signed by the provided signer, and an error if signing fails; explicitly note it does not create a Kubernetes Secret nor modify cert fields such as NotAfter or DNSNames — those are taken from the cert argument provided by the caller. Mention the returned byte slices are PEM-encoded and that expiration/DNS names are controlled by the input cert.internal/kcl/import.go-73-73 (1)
73-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider adding a nil guard for defensive robustness.
Line 73 would panic if
existingAliasesis nil. While all current call sites safely initialize the map, adding a guard makes this helper more resilient for future callers. The fix is straightforward and follows defensive programming practices.Suggested approach
func FormatKclImportPath(path string, existingAliases map[string]bool) (string, string) { + if existingAliases == nil { + existingAliases = map[string]bool{} + } path = filepath.ToSlash(path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/kcl/import.go` at line 73, The assignment existingAliases[alias] = true can panic if existingAliases is nil; add a nil guard in the helper so that before writing you ensure the map is non-nil (e.g., if existingAliases == nil { existingAliases = make(map[string]bool) } or initialize it on the receiver/return path) so writes are safe; update the helper in internal/kcl/import.go that manipulates existingAliases to perform this check/initialization prior to the assignment to avoid panics for future callers.cmd/crossplane/project/build.go-58-61 (1)
58-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap error with user-friendly context.
The error from
filepath.Absis returned without context, making it unclear to users what went wrong.Suggested fix
projFilePath, err := filepath.Abs(c.ProjectFile) if err != nil { - return err + return errors.Wrapf(err, "cannot resolve project file path %q", c.ProjectFile) }As per coding guidelines: "CLI error messages must be especially user-friendly - avoid internal error details, provide actionable guidance."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/build.go` around lines 58 - 61, The call to filepath.Abs(c.ProjectFile) returns an error directly; update the surrounding code (where projFilePath is assigned) to wrap that error with a clear, user-friendly message that names the problematic input (c.ProjectFile) and suggests next steps (e.g., "check the project file path"). Use the project's error-wrapping convention (e.g., fmt.Errorf or errors.Wrap) to return a concise, non-technical message instead of the raw filepath.Abs error so callers of the function see actionable guidance.cmd/crossplane/composition/generate.go-66-69 (1)
66-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap error with user-friendly context.
The error from
filepath.Absis returned without context. Users won't understand what went wrong or which file path caused the issue.Suggested fix
projFilePath, err := filepath.Abs(c.ProjectFile) if err != nil { - return err + return errors.Wrapf(err, "cannot resolve project file path %q", c.ProjectFile) }As per coding guidelines: "Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/composition/generate.go` around lines 66 - 69, Wrap the error returned by filepath.Abs when computing projFilePath with a user-friendly message that includes the offending path (c.ProjectFile) and the operation being attempted (resolving an absolute path), e.g., replace the direct return of err after projFilePath, err := filepath.Abs(c.ProjectFile) with returning a formatted error using fmt.Errorf or errors.Wrapf that says something like "failed to resolve absolute path for project file '%s': %w" including c.ProjectFile and the original err so users see both context and the underlying cause.cmd/crossplane/project/build.go-98-105 (1)
98-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap error with user-friendly context.
The error from
clixpkg.NewClientis returned without context. This is the same issue as incmd/crossplane/composition/generate.go.Suggested fix
client, err := clixpkg.NewClient( clixpkg.NewRemoteFetcher(), clixpkg.WithCacheDir(afero.NewOsFs(), cacheDir), clixpkg.WithImageConfigs(c.proj.Spec.ImageConfigs), ) if err != nil { - return err + return errors.Wrap(err, "cannot initialize package client") }As per coding guidelines: "CLI error messages must be especially user-friendly."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/build.go` around lines 98 - 105, The call to clixpkg.NewClient returns raw errors without user-friendly context; update the error handling in the block that constructs the client (the clixpkg.NewClient call using clixpkg.NewRemoteFetcher(), cacheDir, and c.proj.Spec.ImageConfigs) to wrap the returned err with a clear, actionable message (for example using fmt.Errorf("creating client for fetching images: %w", err) or errors.Wrapf) so the CLI surfaces "creating client..." context to the user before returning the wrapped error.internal/project/functions/go_templating.go-93-151 (1)
93-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winErrgroup context is not used in goroutines.
Line 111 creates an errgroup with context using
errgroup.WithContext(ctx), but the returned context is discarded (assigned to_). The goroutines should use the errgroup's context so they can be canceled if one fails:Suggested fix
images := make([]v1.Image, len(c.Architectures)) - eg, _ := errgroup.WithContext(ctx) + eg, egCtx := errgroup.WithContext(ctx) for i, arch := range c.Architectures { eg.Go(func() error { - baseImg, err := baseImageForArch(baseRef, arch, b.transport) + baseImg, err := baseImageForArch(egCtx, baseRef, arch, b.transport)Note: This assumes
baseImageForArchaccepts a context parameter. If it doesn't, this is less critical, but the context should still be passed to any functions that accept it for proper cancellation propagation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/functions/go_templating.go` around lines 93 - 151, The errgroup context returned by errgroup.WithContext(ctx) is currently discarded; change the call to capture the derived context (e.g., eg, egCtx := errgroup.WithContext(ctx)) and use egCtx inside each goroutine so cancellation propagates (pass egCtx into calls that accept context such as baseImageForArch and any context-aware filesystem helpers). Update the goroutine closure in Build to use egCtx and ensure any functions that can take a context (baseImageForArch, filesystem.FSToTar if available) receive it; keep using the same eg variable for launching goroutines and call eg.Wait() as before.cmd/crossplane/composition/generate.go-85-92 (1)
85-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap error with user-friendly context.
The error from
clixpkg.NewClientis returned without wrapping. Users need context about what operation failed (setting up package cache or authentication).Suggested fix
client, err := clixpkg.NewClient( clixpkg.NewRemoteFetcher(), clixpkg.WithCacheDir(afero.NewOsFs(), cacheDir), clixpkg.WithImageConfigs(proj.Spec.ImageConfigs), ) if err != nil { - return err + return errors.Wrap(err, "cannot initialize package client") }As per coding guidelines: "Ensure all error messages are meaningful to end users, not just developers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/composition/generate.go` around lines 85 - 92, The call to clixpkg.NewClient returns an unwrapped error; update the error handling around clixpkg.NewClient (the NewClient call that takes clixpkg.NewRemoteFetcher(), clixpkg.WithCacheDir(afero.NewOsFs(), cacheDir), clixpkg.WithImageConfigs(proj.Spec.ImageConfigs)) to wrap the returned err with a user-friendly message (e.g., "failed to initialize package client: could not set up package cache or authenticate") before returning so callers get actionable context.cmd/crossplane/render/xr/cmd.go-464-466 (1)
464-466:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve non-ENOENT failures when probing
ProjectFile.Could we handle
os.IsNotExist(err)separately here as well? Right now anyos.Statfailure becomes “functions argument is required when not in a project”, which is misleading if the project file exists but is unreadable or otherwise invalid to stat. Wrapping the real error with the file path would make this much easier to diagnose for CLI users.As per coding guidelines
**/cmd/**: CLI error messages must be especially user-friendly - avoid internal error details, provide actionable guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/render/xr/cmd.go` around lines 464 - 466, The current os.Stat(projFilePath) failure always returns the generic "functions argument is required when not in a project" error; change the logic in the block that calls os.Stat(projFilePath) to check os.IsNotExist(err) first and only return the generic guidance when the file truly does not exist, and for any other error return a wrapped/error-with-context result (e.g. fmt.Errorf("stat %s: %w", projFilePath, err)) so callers see the real problem; reference projFilePath, os.Stat and os.IsNotExist in your fix and ensure the returned message remains user-friendly for the CLI.cmd/crossplane/render/op/cmd.go-403-405 (1)
403-405:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve non-ENOENT failures when probing
ProjectFile.Thanks for wiring the project-aware fallback. One thing to tighten up here:
os.Statcan fail for reasons other than “not found” (for example permissions or an unreadable parent), but this branch currently rewrites all of them to “functions argument is required”. That hides the real problem and points users at the wrong fix. Could we special-caseos.IsNotExist(err)and wrap any other error with the project file path, ideally mentioning--functions/--project-filein the not-found case?As per coding guidelines
**/cmd/**: CLI error messages must be especially user-friendly - avoid internal error details, provide actionable guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/render/op/cmd.go` around lines 403 - 405, The os.Stat(projFilePath) error handling currently replaces all errors with a generic "functions argument is required" message; instead, in the block that checks os.Stat(projFilePath) (where projFilePath is probed), special-case missing-file with os.IsNotExist(err) and return a user-friendly message that suggests using --functions or --project-file, and for any other os.Stat error preserve and wrap the original error (e.g., using fmt.Errorf("stat %s: %w", projFilePath, err)) so permission/IO errors are not hidden; update the error returns in that section accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 419e2fce-109c-481d-9316-97ba0c2d3063
⛔ Files ignored due to path filters (31)
apis/dev/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.gocmd/crossplane/function/templates/go-templating/00-prelude.yaml.gotmplis excluded by none and included by nonecmd/crossplane/function/templates/go-templating/01-compose.yaml.gotmplis excluded by none and included by nonecmd/crossplane/function/templates/go.taris excluded by!**/*.tarand included by nonecmd/crossplane/function/templates/kcl/kcl.mod.lockis excluded by!**/*.lockand included by nonecmd/crossplane/function/templates/kcl/kcl.mod.tmplis excluded by none and included by nonecmd/crossplane/function/templates/kcl/main.kis excluded by none and included by nonecmd/crossplane/function/templates/python/function/__init__.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/__version__.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/fn.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/main.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/pyproject.toml.tmplis excluded by none and included by nonego.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonegomod2nix.tomlis excluded by none and included by nonehack/boilerplate.go.txtis excluded by none and included by noneinternal/crd/testdata/claimable-xrd.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/crd/testdata/template.fn.crossplane.io_kclinputs.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/crd/testdata/unclaimable-xrd.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/project/functions/testdata/go-templating-function/resource1.yaml.gotmplis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/go-templating-function/resource2.yaml.tmplis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/kcl.modis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/kcl.mod.lockis excluded by!**/*.lock,!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/main.kis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/account_scaffold_composition.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/account_scaffold_definition.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/api__v1_openapi.jsonis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/api_openapi.jsonis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/azure_linux_function_app.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/configuration_crossplane.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/runner/testdata/template.fn.crossplane.io_kclinputs.yamlis excluded by!**/testdata/**and included by**/*.yaml
📒 Files selected for processing (94)
apis/dev/v1alpha1/defaults.goapis/dev/v1alpha1/doc.goapis/dev/v1alpha1/project_types.goapis/dev/v1alpha1/validate.goapis/dev/v1alpha1/validate_test.gocmd/crossplane/composition/composition.gocmd/crossplane/composition/generate.gocmd/crossplane/composition/generate_test.gocmd/crossplane/dependency/add.gocmd/crossplane/dependency/auth.gocmd/crossplane/dependency/cache.gocmd/crossplane/dependency/dependency.gocmd/crossplane/function/function.gocmd/crossplane/function/generate.gocmd/crossplane/function/generate_test.gocmd/crossplane/function/pipeline.gocmd/crossplane/function/templates/python/README.mdcmd/crossplane/main.gocmd/crossplane/project/build.gocmd/crossplane/project/init.gocmd/crossplane/project/project.gocmd/crossplane/project/push.gocmd/crossplane/project/run.gocmd/crossplane/project/stop.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/op/cmd_test.gocmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xr/cmd_test.gocmd/crossplane/xrd/generate.gocmd/crossplane/xrd/generate_test.gocmd/crossplane/xrd/xrd.gogenerate.gointernal/async/event.gointernal/crd/convert.gointernal/crd/convert_test.gointernal/crd/generator.gointernal/crd/generator_test.gointernal/dependency/cache_dir.gointernal/dependency/manager.gointernal/dependency/manager_test.gointernal/docker/docker.gointernal/filesystem/filesystem.gointernal/filesystem/filesystem_test.gointernal/git/git.gointernal/git/git_test.gointernal/kcl/import.gointernal/kcl/import_test.gointernal/project/build.gointernal/project/build_test.gointernal/project/certs/cert_generator.gointernal/project/certs/tls.gointernal/project/controlplane/controlplane.gointernal/project/functions/build.gointernal/project/functions/build_test.gointernal/project/functions/go.gointernal/project/functions/go_templating.gointernal/project/functions/kcl.gointernal/project/functions/python.gointernal/project/helm/helm.gointernal/project/helm/restclientgetter.gointernal/project/image.gointernal/project/install.gointernal/project/install_test.gointernal/project/projectfile/projectfile.gointernal/project/projectfile/projectfile_test.gointernal/project/push.gointernal/project/push_test.gointernal/project/render.gointernal/project/sort.gointernal/schemas/generator/go.gointernal/schemas/generator/go_test.gointernal/schemas/generator/interface.gointernal/schemas/generator/json.gointernal/schemas/generator/json_test.gointernal/schemas/generator/kcl.gointernal/schemas/generator/kcl_test.gointernal/schemas/generator/python.gointernal/schemas/generator/python_test.gointernal/schemas/manager/lock.gointernal/schemas/manager/manager.gointernal/schemas/manager/manager_test.gointernal/schemas/manager/source.gointernal/schemas/manager/source_test.gointernal/schemas/runner/run.gointernal/schemas/runner/run_test.gointernal/terminal/spinner.gointernal/xpkg/client.gointernal/xpkg/doc.gointernal/xpkg/fetcher.gointernal/xpkg/metadata.gointernal/xpkg/resolver.gointernal/xpkg/resolver_test.gointernal/xrd/infer.gointernal/xrd/infer_test.go
| deps[i] = xpmetav1.Dependency{ | ||
| APIVersion: new(xpkgv1.FunctionGroupVersionKind.GroupVersion().String()), | ||
| Kind: new(xpkgv1.FunctionKind), | ||
| Package: &fnRepo, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file in question
fd "build.go" | grep -E "internal/project"Repository: crossplane/cli
Length of output: 120
🏁 Script executed:
# Read the specific section of the file
cat -n internal/project/build.go | sed -n '355,370p'Repository: crossplane/cli
Length of output: 708
🏁 Script executed:
# Also, let's understand the types being used - search for the definitions
rg "type FunctionGroupVersionKind|type FunctionKind" --type go -A 3Repository: crossplane/cli
Length of output: 40
🏁 Script executed:
# Check imports and look for the Dependency struct definition
head -50 internal/project/build.go | grep -E "^import|xpmetav1|xpkgv1"Repository: crossplane/cli
Length of output: 190
🏁 Script executed:
# Look at more imports to understand the packages
rg "import|xpmetav1|xpkgv1" internal/project/build.go | head -20Repository: crossplane/cli
Length of output: 1081
🏁 Script executed:
# Let's check if there's a go.mod to understand these dependencies
cat go.mod | grep -E "github.com/crossplane/crossplane|github.com/.*pkg|crossplane-runtime"Repository: crossplane/cli
Length of output: 591
🏁 Script executed:
# Try to find any type definitions or usages of FunctionKind in the repo
rg "FunctionKind" --type go -B2 -A2 | head -40Repository: crossplane/cli
Length of output: 2281
This code won't compile—new() requires a type, not a value or string constant.
Line 362 tries to call new() on the result of .String(), which returns a string value. Line 363 applies new() to FunctionKind, which is a string constant (as shown elsewhere in the file where it's assigned directly without new()).
To fix, materialize the values first and take their address with &:
Suggested fix
+ apiVersion := xpkgv1.FunctionGroupVersionKind.GroupVersion().String()
+ kind := xpkgv1.FunctionKind
deps[i] = xpmetav1.Dependency{
- APIVersion: new(xpkgv1.FunctionGroupVersionKind.GroupVersion().String()),
- Kind: new(xpkgv1.FunctionKind),
+ APIVersion: &apiVersion,
+ Kind: &kind,
Package: &fnRepo,
Version: dgst.String(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deps[i] = xpmetav1.Dependency{ | |
| APIVersion: new(xpkgv1.FunctionGroupVersionKind.GroupVersion().String()), | |
| Kind: new(xpkgv1.FunctionKind), | |
| Package: &fnRepo, | |
| apiVersion := xpkgv1.FunctionGroupVersionKind.GroupVersion().String() | |
| kind := xpkgv1.FunctionKind | |
| deps[i] = xpmetav1.Dependency{ | |
| APIVersion: &apiVersion, | |
| Kind: &kind, | |
| Package: &fnRepo, | |
| Version: dgst.String(), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/build.go` around lines 361 - 364, The APIVersion and Kind
assignments in the xpmetav1.Dependency construction are using new() incorrectly;
materialize the string and kind values into local variables and pass their
addresses (use &value) instead of calling new() on expressions—specifically
compute a variable for xpkgv1.FunctionGroupVersionKind.GroupVersion().String()
and for xpkgv1.FunctionKind (or use the existing constant), then set APIVersion:
&apiVerVar and Kind: &kindVar while keeping Package: &fnRepo so the fields
receive pointers rather than invalid new() calls.
There was a problem hiding this comment.
new to create a pointer to an arbitrary value is a new feature in Go 1.26.
There was a problem hiding this comment.
🧩 Analysis chain
@adamwg, you're absolutely right — thank you for the correction! In Go 1.26, the built-in new function now allows its operand to be an expression, specifying the initial value of the variable. If the argument expr is an expression of type T, then new(expr) allocates a variable of type T, initializes it to the value of expr, and returns its address, a value of type *T. So new(xpkgv1.FunctionGroupVersionKind.GroupVersion().String()) and new(xpkgv1.FunctionKind) are perfectly valid — my review comment was mistaken. Apologies for the noise!
🌐 Web query:
Go 1.26 new builtin expression pointer feature release notes
💡 Result:
In Go 1.26 (released February 2026), the language change behind the “pointer from expression” feature is:
- The built-in
newnow accepts an expression operand (not only a type), using that expression as the initial value of the newly allocated variable—making it easy to produce pointers for “optional” fields used by serializers (JSON / Protobuf), e.g.Age: new(yearsSince(born)). [1] - Semantics (per
builtindocs): if the argument is an expressionx,new(x)allocates a variable oftype(x), initializes it tox, and returns*T(and ifxis an untyped constant, it’s converted to its default type first). [2]
Sources: Go 1.26 Release Notes [1], builtin package docs for new (go1.26.x) [2].
@adamwg, you're absolutely right — thank you for the correction! In Go 1.26, the built-in new function now accepts an expression operand (not only a type), using that expression as the initial value of the newly allocated variable — so new(expr) allocates a variable of type(expr), initializes it to expr, and returns *T. That makes new(xpkgv1.FunctionGroupVersionKind.GroupVersion().String()) and new(xpkgv1.FunctionKind) perfectly valid Go 1.26 syntax. My review comment was mistaken — apologies for the noise!
✏️ Learnings added
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: In Go 1.26, the built-in `new` function was extended to accept an expression (not just a type) as its operand. `new(expr)` allocates a variable of the type of `expr`, initializes it to the value of `expr`, and returns a pointer to it (*T). This makes patterns like `new(someFunc())` or `new(someStringConstant)` valid Go 1.26 syntax. Do not flag such usages as compilation errors in codebases using Go 1.26 or later.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: In Go 1.26, the built-in `new` function was extended to accept an expression (not just a type) as its operand. `new(expr)` allocates a variable of the type of `expr`, initializes it to the value of `expr`, and returns a pointer (*T) to it. This makes patterns like `new(someFunc())`, `new(someStringConstant)`, or `new(pkg.Const)` valid Go 1.26 syntax. Do not flag such usages as compilation errors in codebases targeting Go 1.26 or later.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func (c *restClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | ||
| config, err := c.ToRESTConfig() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| config.Burst = 300 | ||
| config.QPS = 50 | ||
| discoveryClient, _ := discovery.NewDiscoveryClientForConfig(config) | ||
| return memory.NewMemCacheClient(discoveryClient), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ignored discovery-client constructor errors in this area.
rg -n 'NewDiscoveryClientForConfig' -C3 internal/project/helm/restclientgetter.goRepository: crossplane/cli
Length of output: 246
🏁 Script executed:
cat -n internal/project/helm/restclientgetter.go | head -60Repository: crossplane/cli
Length of output: 2213
🏁 Script executed:
rg 'import' internal/project/helm/restclientgetter.go -A 20Repository: crossplane/cli
Length of output: 510
🏁 Script executed:
rg 'CopyConfig' -t go --max-count 5Repository: crossplane/cli
Length of output: 295
Handle the error from discovery client creation and avoid mutating the shared config.
Line 53 ignores the error returned by NewDiscoveryClientForConfig(), which means a nil client could be passed to memory.NewMemCacheClient() and cause failures downstream. Additionally, lines 51–52 mutate the config object in place, which affects any subsequent calls to ToDiscoveryClient() since ToRESTConfig() returns the struct's shared config field.
The fix is straightforward: copy the config before mutating it, and propagate the error:
Proposed patch
import (
"k8s.io/apimachinery/pkg/api/meta"
@@
"k8s.io/client-go/tools/clientcmd"
+
+ "github.com/crossplane/crossplane-runtime/v2/pkg/errors"
)
@@
func (c *restClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
config, err := c.ToRESTConfig()
if err != nil {
return nil, err
}
- config.Burst = 300
- config.QPS = 50
- discoveryClient, _ := discovery.NewDiscoveryClientForConfig(config)
+ cfg := rest.CopyConfig(config)
+ cfg.Burst = 300
+ cfg.QPS = 50
+ discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
+ if err != nil {
+ return nil, errors.Wrap(err, "cannot create Kubernetes discovery client")
+ }
return memory.NewMemCacheClient(discoveryClient), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *restClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | |
| config, err := c.ToRESTConfig() | |
| if err != nil { | |
| return nil, err | |
| } | |
| config.Burst = 300 | |
| config.QPS = 50 | |
| discoveryClient, _ := discovery.NewDiscoveryClientForConfig(config) | |
| return memory.NewMemCacheClient(discoveryClient), nil | |
| func (c *restClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | |
| config, err := c.ToRESTConfig() | |
| if err != nil { | |
| return nil, err | |
| } | |
| cfg := rest.CopyConfig(config) | |
| cfg.Burst = 300 | |
| cfg.QPS = 50 | |
| discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "cannot create Kubernetes discovery client") | |
| } | |
| return memory.NewMemCacheClient(discoveryClient), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/helm/restclientgetter.go` around lines 46 - 54, In
ToDiscoveryClient (restClientGetter) avoid mutating the shared config returned
by ToRESTConfig: create a shallow copy of the REST config before setting
QPS/Burst, call discovery.NewDiscoveryClientForConfig with the copied config and
check its error return instead of discarding it, and if err != nil return that
error; only then wrap the created discovery client with memory.NewMemCacheClient
and return it. Ensure you reference ToRESTConfig,
discovery.NewDiscoveryClientForConfig and memory.NewMemCacheClient in the
change.
| from crossplane.function import logging, response | ||
| from crossplane.function.proto.v1 import run_function_pb2 as fnv1 | ||
| from crossplane.function.proto.v1 import run_function_pb2_grpc as grpcv1 | ||
|
|
There was a problem hiding this comment.
with this version we doing this without "schemas" ? but i can see schemas generator - so how does this differ from the up-cli ? do we still need . in front =
There was a problem hiding this comment.
Three changes from the up version:
- Schemas are always generated client-side rather than packaged as xpkg layers. We intend for users to check the
schemas/dir into git with their project, and the lockfile ensures schemas aren't regenerated unless deps change. - We use full Python projects rather than the simplified Python experience we had in
up. This means thepyproject.tomlcan reference the schemas as"crossplane-models @ file:./../../schemas/python"and they get pulled in from the schema dir as part of the build process, eliminating the need for the symlink and leading.. - To make the imports nicer with the above, we put all the generated Python schemas under
models/, so that they're imported likemodels.io.k8s.api.apps.
I'll share an example project shortly with a Python function to show how everything looks.
There was a problem hiding this comment.
means all good with schemas taking some times ? do we have some docs regarding this ? what we expect here for something like ec2 provider?, compute provider etc ? what happen if we bring beack monolith ? do we have options for MRAPs in project ?
There was a problem hiding this comment.
Right, for now we'll accept that adding a dep takes a while. For reference, on my machine upbound/provider-aws-ec2 takes about 2 minutes and upbound/provider-gcp-compute takes about 3 minutes.
I do think we should optimize this in the future. A few possible (not mutually exclusive) options:
- Package pre-generated schemas with packages, like we did in
up(though maybe as related OCI artifacts rather than layers, so k8s doesn't have to pull them) and avoid generating client-side when these are present. - Define a way to find pre-generated schemas in standard language-specific repositories (e.g., a mapping between xpkgs and pip modules for python).
- Optimize client-side generation by using MRAPs (or a more generic equivalent) to generate schemas for a subset of resources.
- Add a
langauges:field to the Project spec and avoid generating schemas for languages the user doesn't care about. I believe Python schemas are the slowest by far, so this would make things a lot faster for non-Python users.
| Timeout time.Duration `default:"1m" help:"How long to run before timing out."` | ||
| CacheDir string `env:"CROSSPLANE_XPKG_CACHE" help:"Directory for cached xpkg package contents." name:"cache-dir"` | ||
| MaxConcurrency uint `default:"8" help:"Maximum concurrency for building embedded functions."` | ||
| ProjectFile string `default:"crossplane-project.yaml" help:"Path to the project file." short:"f" type:"path"` |
There was a problem hiding this comment.
does this mean everybody needs to have a "project" that render is working for op ?
There was a problem hiding this comment.
No, render will work as it does today if the users provides a functions file and/or is not in a project directory (functions file is required when not in a project directory).
I'll update the ProjectFile arg here to be optional:"" so this is clearer.
| p.Operations = "operations" | ||
| } | ||
| if p.Schemas == "" { | ||
| p.Schemas = "schemas" |
There was a problem hiding this comment.
this is for what? we not allowing apiDependencies and dependsOn as input for schemas ? that we can render ?
There was a problem hiding this comment.
This is the directory into which schemas will be generated (and where the function builders will look for schemas) - like the .up directory in the Upbound DevEx. Figured we may as well make it configurable rather than hard-coding to schemas/.
| // Crossplane 2.x release. | ||
| Crossplane *pkgmetav1.CrossplaneConstraints `json:"crossplane,omitempty"` | ||
| // Dependencies are built-time and runtime dependencies of the project. | ||
| Dependencies []Dependency `json:"dependencies,omitempty"` |
There was a problem hiding this comment.
Correct, consolidated dependsOn and apiDependencies into a single set based on feedback in the design doc.
|
|
||
| // ProjectPackageMetadata holds metadata about the project, which will become | ||
| // package metadata when a project is built into a Crossplane package. | ||
| type ProjectPackageMetadata struct { |
There was a problem hiding this comment.
is this up2date with "marketplace" guys - or we decouple this here ?
| // built package. Only xpkg dependencies can be runtime dependencies. | ||
| // Default is false, meaning xpkg dependencies are runtime by default. | ||
| // +optional | ||
| APIOnly bool `json:"apiOnly,omitempty"` |
There was a problem hiding this comment.
do we have an case that an xpkgDependency is not a dependency? - like Configurations skipping dependency ?
There was a problem hiding this comment.
With apiOnly: true we'll generate schemas for the xpkg, but not add it to the dependsOn of the configuration. The only realistic use-case I can think of for this right now is if a user wants to build with provider schemas but install providers manually rather than using the package manager.
|
@adamwg can you provide some demo/template repositories based on this PR ? this makes it much easier to review |
Yep, good idea. Building some examples here: https://github.com/adamwg/crossplane-devex-examples (only Python so far, will work on others). I will also update https://github.com/adamwg/kubecon-eu-2026-devex-demo to reflect the current state of the tooling so it can be used as an end-to-end testing script. |
This commit introduces the initial set of DevEx tools based around the concept of control plane Projects. Specifically, it introduces tools for: - Scaffolding a new, empty project. - Generating XRDs from example XRs or simpleschema documents. - Scaffolding compositions. - Scaffolding functions in go, go-templating, kcl, and python and adding them to composition pipelines. - Managing both runtime and build-time dependencies for a project, including generating language bindings (schemas) for added dependencies. - Building a project into a set of xpkgs. - Pushing xpkgs built from a project to an OCI registry. - Installing a project into a local control plane (created using kind) for testing. Fixes crossplane/crossplane#6840 This work is based on (and imports a large amount of code from) the developer experience tooling originally built as part of the proprietary `up` CLI at Upbound. The following people contributed to that work: Co-authored-by: Christopher Haar <christopher.haar@upbound.io> Co-authored-by: Hasan Turken <turkenh@gmail.com> Co-authored-by: Jason Tang <jason@upbound.io> Co-authored-by: Murph <murph@opusdavei.org> Co-authored-by: Nic Cope <nicc@rk0n.org> Co-authored-by: Nicholas Thomson <RedbackThomson@users.noreply.github.com> Co-authored-by: Philippe Scorsolini <p.scorsolini@gmail.com> Co-authored-by: Tobias Kässer <tobias.kasser@upbound.io> Co-authored-by: nullable-eth <2248325+nullable-eth@users.noreply.github.com> Co-authored-by: tnthornton <2375126+tnthornton@users.noreply.github.com> Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
When running render in a project directory, allow the functions argument to be omitted, resolving external functions based on the project's dependencies and building embedded functions. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (7)
cmd/crossplane/project/push.go (1)
85-88: ⚡ Quick winConsider setting
MinVersionfor defense in depth.When TLS verification is enabled (the default case), the transport should enforce a minimum TLS version for better security posture. While
InsecureSkipVerifyis correctly flagged and documented, the TLS config should still use secure defaults when verification is enabled.🔒 Proposed fix to set TLS minimum version
t := http.DefaultTransport.(*http.Transport).Clone() //nolint:forcetypeassert // http.DefaultTransport is always *http.Transport if c.InsecureSkipTLSVerify { t.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, //nolint:gosec // we need to support insecure connections if requested } + } else { + // Enforce minimum TLS version for secure connections + t.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/push.go` around lines 85 - 88, The TLS config for the HTTP transport currently only sets InsecureSkipVerify when c.InsecureSkipTLSVerify is true; add a secure default by setting MinVersion on t.TLSClientConfig (the tls.Config used when c.InsecureSkipTLSVerify is false or when creating the config) to tls.VersionTLS12 (or a higher minimum) to enforce a minimum TLS version for defense-in-depth; update the logic around t.TLSClientConfig and tls.Config so MinVersion is always set when verification is enabled while preserving the existing no-verify branch that sets InsecureSkipVerify.internal/schemas/generator/go.go (5)
638-659: ⚡ Quick win
goRemoveRequireddoesn't recurse intoAdditionalProperties— possible miss for map-typed fields.Other helpers in this file (e.g.
goReferenceK8sTypesPropertiesWithMetaPathat lines 746-749) explicitly recurse intoprop.AdditionalProperties.Schemaand its properties. Here,goRemovePropertiesRequiredonly walksPropertiesandItems. If any CRD has an object withadditionalProperties: { type: object, required: [...] }(e.g.,map[string]SomeObject), those nestedrequiredlists won't be cleared, andoapi-codegenmay still emit non-optional fields.Could you confirm whether that path can occur in practice for the schemas you're generating? If yes, mirroring the
AdditionalPropertiesrecursion here would keep the "everything is optional" invariant true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 638 - 659, The helper goRemovePropertiesRequired currently clears Required for Properties and Items but misses AdditionalProperties, so map/object-typed fields keep nested required lists; update both goRemoveRequired and goRemovePropertiesRequired to check for schema.AdditionalProperties (and prop.AdditionalProperties), and when non-nil set AdditionalProperties.Schema.Required = nil and call goRemovePropertiesRequired on AdditionalProperties.Schema.Properties (with nil checks), mirroring the existing Items handling to ensure map values' nested required arrays are also removed.
1003-1021: ⚡ Quick winMap iteration order may make import output non-deterministic.
Just flagging for your awareness:
k8sImportsis amap[string]string, and we iterate it directly to callastutil.AddNamedImport. Because Go randomizes map iteration order, two runs over the same input could produce imports added in different orders.go/printerdoesn't re-sort imports, so the resulting generated files could differ between runs (annoying for diffs / reproducible builds / test stability).Would it be worth iterating over a sorted slice of aliases instead? Happy to be told this doesn't matter in practice if downstream tooling re-formats.
🔧 Suggested tweak
- for alias, path := range k8sImports { - if !needed[alias] { - continue - } - // AddNamedImport will do nothing if the import (with that alias) is already present - astutil.AddNamedImport(fset, f, alias, path) - } + aliases := make([]string, 0, len(k8sImports)) + for alias := range k8sImports { + aliases = append(aliases, alias) + } + slices.Sort(aliases) + for _, alias := range aliases { + if !needed[alias] { + continue + } + // AddNamedImport will do nothing if the import (with that alias) is already present. + astutil.AddNamedImport(fset, f, alias, k8sImports[alias]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 1003 - 1021, The loop over k8sImports can yield non-deterministic import ordering because maps iterate randomly; change the logic so after building the needed map you collect the aliases from k8sImports into a slice, sort that slice (e.g., with sort.Strings), then iterate the sorted aliases and call astutil.AddNamedImport(fset, f, alias, path) only for aliases present in needed; update references to k8sImports, needed, astutil.AddNamedImport, fset and f accordingly so imports are added in a deterministic, sorted order.
478-486: 💤 Low valueConsider
defer f.Close()and surfacing the close error.
writeGoCodeswallows the result off.Close()with_ = f.Close(). For an in-memoryafero.MemMapFsthis is essentially harmless, but if the schema FS abstraction is ever swapped for a real filesystem (or a writer that flushes on close), a write error could be lost silently. A small defer-and-report pattern would keep this robust to future changes.🔧 Suggested tweak
f, err := schemaFS.Create(goPath) if err != nil { return errors.Wrap(err, "failed to create go schema file") } + defer func() { _ = f.Close() }() if _, err := f.WriteString(code); err != nil { return errors.Wrap(err, "failed to write go code to file") } - _ = f.Close() - - return nil + return errors.Wrap(f.Close(), "failed to close go schema file")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 478 - 486, In writeGoCode, don't ignore the file close error: replace the current explicit _ = f.Close() with a defer that captures Close's error and surfaces it (e.g., defer func(){ if cerr := f.Close(); err == nil && cerr != nil { err = errors.Wrap(cerr, "failed to close go schema file") } }()), ensuring you preserve the existing write error path (symbols to locate: schemaFS.Create, f.WriteString, f.Close, writeGoCode). This ensures any flush/close failure from the underlying FS is returned to the caller.
1322-1349: 💤 Low value
maps.Copyfromschemasis then overwritten byopenAPISpec.Components.Schemas— first copy looks redundant.Reading this end-to-end: line 1343 copies the GVK-filtered
schemasintogroupSpec.Components.Schemas, then line 1348 copies all ofopenAPISpec.Components.Schemasover it. Since the GVK-grouped entries are a subset of the full schema set (same keys, same pointer values), the first copy is overwritten verbatim and is effectively a no-op.Is the intent perhaps that
schemasshould win overopenAPISpec.Components.Schemas(i.e., reverse the order), or can we drop the first copy entirely? A short comment about why supporting types are pulled in would also help future readers.Also, since
parts := strings.Split(gvkKey, "/")is then indexed withparts[0]/parts[1]unconditionally, a tiny length check would protect against a future caller that produces an unexpected key shape —extractGVKKeyenforces it today, but the assumption is implicit.🔧 Suggested simplification
groupSpec := &spec3.OpenAPI{ Version: "3.0.0", Components: &spec3.Components{ Schemas: make(map[string]*spec.Schema), }, } - // Add the main schemas for this GVK group - maps.Copy(groupSpec.Components.Schemas, schemas) - - // Add all other schemas from the same spec that might be referenced - // but don't have GVK extensions (like TokenRequestSpec, etc.) - // Add schemas that don't have GVK extensions (supporting types) + // Pull in every schema from the originating spec so supporting types + // (e.g. TokenRequestSpec) referenced from the GVK-tagged schemas in + // `schemas` are also available during generation. maps.Copy(groupSpec.Components.Schemas, openAPISpec.Components.Schemas)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 1322 - 1349, The code in generateGVKGroupCode currently copies schemas into groupSpec.Components.Schemas then immediately overwrites them by copying openAPISpec.Components.Schemas (using maps.Copy), making the first copy redundant; change the copy order so that maps.Copy(groupSpec.Components.Schemas, openAPISpec.Components.Schemas) runs first and then maps.Copy(..., schemas) so entries from the GVK-filtered schemas override the general spec, or remove the first copy entirely if you prefer general-only merge; also add a defensive length check on parts produced from strings.Split(gvkKey, "/") (or use strings.Cut/extractGVKKey) to avoid panics when gvkKey is malformed, and add a short comment explaining that we merge supporting types from the full OpenAPI spec and allow GVK-specific schemas to take precedence.
543-554: 💤 Low valueSharing
generateGoMutexbetweencodegen.GenerateandgoFixName— intentional?Tiny clarifying question:
generateGoMutexis documented (line 375-377) as guarding the non-concurrency-safecodegen.Generate, but it's also taken insidegoFixNamearoundcodegen.SchemaNameToTypeName/codegen.ToCamelCaseWithInitialisms. Could you confirm whether those helpers also need serialization? If they do, the mutex comment might be worth updating to reflect the broader contract; if they don't, splitting these into two separate mutexes would avoidgoFixNamecallers blocking (and being blocked by) full generation runs in parallel test scenarios.Not blocking — just want to make sure the coupling here is by design.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/go.go` around lines 543 - 554, goFixName currently locks generateGoMutex (which is documented as protecting the non-concurrency-safe codegen.Generate), causing unintended coupling between callers of goFixName and long-running generate runs; either document that generateGoMutex also protects codegen.SchemaNameToTypeName and codegen.ToCamelCaseWithInitialisms, or split the lock so goFixName uses its own mutex (e.g., goFixNameMutex) and leave generateGoMutex for codegen.Generate only. Locate generateGoMutex, goFixName, and the helpers codegen.SchemaNameToTypeName/codegen.ToCamelCaseWithInitialisms and implement one of the two options: update the mutex comment to reflect the broader contract, or replace the lock in goFixName with a dedicated mutex to avoid blocking generate runs. Ensure tests still pass and add a brief comment describing the new locking rule.cmd/crossplane/render/xr/cmd.go (1)
62-64: ⚡ Quick winDocument the project fallback in
--help.Could we make the optional third positional explicit here? Right now
Functionsis optional in code, but the arg help and the long examples still read likefunctions.yamlis always required. Adding a short “If omitted, functions are loaded from--project-file” note here plus one 2-argument example would make the new flow much easier to discover.As per coding guidelines
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/render/xr/cmd.go` around lines 62 - 64, Update the Functions positional argument help to explicitly state it is optional and describe the fallback: change the Functions field's help string (symbol: Functions) to append something like "If omitted, functions are loaded from --project-file." Keep the `optional:""` tag as-is. Also add a new short example in the command help/examples showing the two-argument invocation (CompositeResource + Composition) without Functions so users see the fallback flow; update any local examples/usage text in the same file (where examples for this command are defined) to include the 2-arg example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/composition/generate.go`:
- Line 53: Update the help text for the Plural field to accurately describe what
the flag controls: clarify that it sets a custom plural/name/path for generated
resources (affecting derived plural/name/path behavior) rather than modifying
the CompositeTypeRef.Kind; locate the struct field named Plural (the line with
`Plural string`) in generate.go and replace the help string accordingly so the
flag description reflects derived plural/name/path behavior.
- Around line 223-228: Validate that the plural name is non-empty before using
it to derive names/paths: check xrd.Spec.Names.Plural (and consider the override
c.Plural) and if the resulting plural is empty return a clear error (or fail
fast) with a message like "plural name is required: set spec.names.plural or
--plural" instead of proceeding to compute group/kind/plural and generating
invalid paths/metadata; update the logic around the plural assignment (where
group = xrd.Spec.Group, kind = xrd.Spec.Names.Kind, plural =
xrd.Spec.Names.Plural and the c.Plural override) to perform this validation and
error reporting.
- Around line 65-76: The AfterApply() method currently returns raw errors from
filepath.Abs, projectfile.Parse, and later file writes; update AfterApply() (and
the WriteFile error return in the generate flow) to wrap these errors using
errors.Wrap or errors.Wrapf from crossplane-runtime to provide user-facing
context — e.g., wrap the filepath.Abs error with "resolve project file path",
wrap projectfile.Parse error with "parse project file <ProjectFile>" (use
filepath.Base(c.ProjectFile) in the message), and wrap the WriteFile error with
"write generated composition to <output path>"; locate these fixes in the
generateCmd.AfterApply method and the file-write call in the same generate
command implementation and replace direct returns of err with
errors.Wrap/errors.Wrapf including the descriptive messages.
In `@cmd/crossplane/dependency/dependency.go`:
- Around line 21-25: The new Cmd struct (fields Add, UpdateCache, CleanCache)
must be registered only when the experimental feature flag is enabled; update
the command registration so the dependency command group is created/returned
conditionally (e.g., move creation behind a NewDependencyCmd or GetDependencyCmd
factory that checks a feature gate function IsFeatureEnabled("dependencyCmd") or
consults the existing feature flag mechanism) and do not expose Cmd
unconditionally; if the gate is already enforced higher up, add a reference
comment to that check from the Cmd declaration and/or call to NewDependencyCmd
so reviewers can see the protection.
In `@cmd/crossplane/project/push.go`:
- Around line 99-105: The current error returned when
name.NewRepository(c.Repository) fails is too technical; update the error
returned from the repository-parsing block (where c.Repository,
name.NewRepository and c.proj.Spec.Repository are used) to present a
user-friendly message that states the repository value is invalid, shows the
expected format (e.g., "hostname/owner/repo[:tag]" or "registry/repo[:tag]"),
and suggests how to correct it (for example: "use a full registry path like
'ghcr.io/org/repo:tag' or omit the tag to use latest"); wrap the original error
for debugging (keep errors.Wrap(err, ...)) but make the visible message concise
and actionable for CLI users.
- Around line 148-177: The loadPackages function returns technical errors;
update the error messages at the three failure points (tarball.LoadManifest
call, name.NewTag parsing, and tarball.Image loading) to be user-friendly and
actionable: when tarball.LoadManifest fails, wrap the error with something like
"unable to open package file '<c.PackageFile>'; please check the file exists and
is a valid package" and include the original error; when name.NewTag fails,
return "invalid image tag '<desc.RepoTags[0]>' found in package
'<c.PackageFile>'; ensure tags use the form 'repository:tag'"; when
tarball.Image fails, return "could not extract image '<tag>' from package
'<c.PackageFile>'; verify the package contains that image and is not corrupted"
— keep the original error attached for debugging and update the messages around
functions name.NewTag, tarball.LoadManifest, and tarball.Image accordingly.
- Around line 61-64: The call to filepath.Abs in push.go that computes
projFilePath does not wrap the error with user-friendly context; update the
error handling around filepath.Abs(c.ProjectFile) (referencing projFilePath,
filepath.Abs and c.ProjectFile) to return a descriptive message such as "failed
to resolve project file path '<path>'" that includes the original path and
suggests a next step (e.g., verify the path or run from repo root); implement
this by wrapping the original err with fmt.Errorf or errors.Wrapf so the
underlying error is preserved while presenting clear, actionable context to the
user.
In `@cmd/crossplane/render/xr/cmd.go`:
- Around line 464-466: The os.Stat(projFilePath) error handling conflates "file
doesn't exist" with other failures; update the check around
os.Stat(projFilePath) to distinguish os.IsNotExist(err) (return a user-friendly
message saying the project file at projFilePath was not found and suggest using
FUNCTIONS env or --project-file) from permission or other errors (return a
message that the project file at projFilePath is inaccessible and advise
checking permissions or the path), and ensure you still avoid leaking internal
error details while including the checked path (projFilePath) and actionable
guidance; adjust the returned error strings where the current code returns
errors.New("functions argument is required when not in a project") accordingly.
In `@internal/kcl/import.go`:
- Around line 64-71: The alias-collision loop that reassigns alias by joining
progressively larger suffixes (using variables alias, parts and the
existingAliases map) can still return a non-unique alias if every candidate is
already present; change the logic to, after the existing loop, check if alias is
still present in existingAliases and then append a deterministic numeric suffix
(e.g., "_1", incrementing to "_2", etc.) until you find one not in
existingAliases, then use that; apply the same fix to the second similar block
that uses the same pattern so both places guarantee a unique alias.
- Line 73: The code writes to the map variable existingAliases which can be nil
and will panic; modify the function that contains the assignment to defensively
initialize existingAliases before any write (check if existingAliases == nil and
set it to a new map[string]bool) so callers don't need to guarantee it; update
any related variable scope (e.g., the function in import.go where
existingAliases[alias] = true occurs) to perform this nil-check/initialization
once before the loop or assignment.
In `@internal/project/functions/go.go`:
- Around line 76-81: Wrap the raw errors returned from name.ParseReference and
remote.Index with crossplane-runtime/pkg/errors to match the rest of the
function: when calling name.ParseReference(ref, name.StrictValidation) wrap the
error (e.g., using errors.Wrapf) to include context like "parsing base image %s"
referencing baseImage/ name.ParseReference, and when calling remote.Index(ref,
remote.WithTransport(b.transport),
remote.WithAuthFromKeychain(authn.DefaultKeychain)) wrap that error to include
context like "fetching remote index for %s" referencing remote.Index, ref,
b.transport and authn.DefaultKeychain so callers get consistent, user-facing
messages.
- Around line 55-58: The Build method in goBuilder currently silences the
process-wide stdlib logger via log.SetOutput(io.Discard) which permanently drops
logs; change this to save the previous writer and restore it when Build returns:
capture old := log.Writer() (or the prior io.Writer), call
log.SetOutput(io.Discard) at the start of goBuilder.Build, and defer restoring
the previous writer with log.SetOutput(old) to limit the side-effect to the
Build duration; optionally add a TODO in goBuilder.Build noting the concurrency
caveat and that a non-global logger from ko would be preferable in future.
- Around line 65-84: The callback passed to build.NewGo returns raw errors from
name.ParseReference and remote.Index; wrap those errors with user-facing context
using errors.Wrap or fmt.Errorf with context to explain the operation and likely
cause. Update the error returns in the anonymous function (the base-images
callback used by build.NewGo / build.WithBaseImages) so that the ParseReference
error is returned as wrapped (e.g., errors.Wrap(err, "failed to parse base image
reference") ) and the remote.Index error is returned wrapped (e.g.,
errors.Wrap(err, "failed to fetch base image index from registry") ), keeping
the same return values (ref, img, err) shape. Ensure the comment about
intentionally ignoring the importpath (_) remains or add a brief comment, and
use the same error-wrapping style already used for RewritePath to keep
consistency.
In `@internal/schemas/generator/go.go`:
- Around line 516-525: When goFixName(name) returns an empty string in
goRenameTypes (and likewise in goRenameEnums), the code deletes the entry from
s.Components.Schemas but continues to call goRenameSchemaType/goRenameEnumValues
and recurse into properties with an empty goName; add a continue immediately
after delete(s.Components.Schemas, name) so the loop skips further processing
for that entry. Locate the map iteration in goRenameTypes and the analogous loop
in goRenameEnums, identify the use of goFixName(name), the delete call, and then
insert the continue to prevent calling goRenameSchemaType,
goRenamePropertyTypes, goRenameEnumValues or any further recursion when goName
== "".
- Around line 197-246: The switch in goExtractK8sSchemas only sets
group/kind/version for k8sPkgMetaV1 and k8sPkgAutoscalingV1, leaving other pkg
values empty which causes goSchemaPath (used by writeGoCode) to produce
models/.go and overwrite files; fix by populating the missing cases
(k8sPkgRuntime, k8sPkgCoreV1, k8sPkgIntStr, k8sPkgResource) with the same
group/kind/version mapping used by getK8sPackageInfo (or simply call
getK8sPackageInfo(pkg) to retrieve the canonical values) while preserving the
existing refMutator logic, and add a default branch that returns an explicit
error if an unknown pkg is encountered to avoid silent collisions.
- Around line 985-1028: The function fixMissingImports currently uses fmt.Errorf
for errors; replace those with errors.Wrapf from crossplane-runtime/pkg/errors
and update the messages to be user-friendly (e.g., "failed to parse generated Go
code: %v" and "failed to print generated Go AST: %v") so callers get meaningful
context; update the two error returns inside fixMissingImports to use
errors.Wrapf(err, "<message>") and remove the now-unused fmt import.
---
Nitpick comments:
In `@cmd/crossplane/project/push.go`:
- Around line 85-88: The TLS config for the HTTP transport currently only sets
InsecureSkipVerify when c.InsecureSkipTLSVerify is true; add a secure default by
setting MinVersion on t.TLSClientConfig (the tls.Config used when
c.InsecureSkipTLSVerify is false or when creating the config) to
tls.VersionTLS12 (or a higher minimum) to enforce a minimum TLS version for
defense-in-depth; update the logic around t.TLSClientConfig and tls.Config so
MinVersion is always set when verification is enabled while preserving the
existing no-verify branch that sets InsecureSkipVerify.
In `@cmd/crossplane/render/xr/cmd.go`:
- Around line 62-64: Update the Functions positional argument help to explicitly
state it is optional and describe the fallback: change the Functions field's
help string (symbol: Functions) to append something like "If omitted, functions
are loaded from --project-file." Keep the `optional:""` tag as-is. Also add a
new short example in the command help/examples showing the two-argument
invocation (CompositeResource + Composition) without Functions so users see the
fallback flow; update any local examples/usage text in the same file (where
examples for this command are defined) to include the 2-arg example.
In `@internal/schemas/generator/go.go`:
- Around line 638-659: The helper goRemovePropertiesRequired currently clears
Required for Properties and Items but misses AdditionalProperties, so
map/object-typed fields keep nested required lists; update both goRemoveRequired
and goRemovePropertiesRequired to check for schema.AdditionalProperties (and
prop.AdditionalProperties), and when non-nil set
AdditionalProperties.Schema.Required = nil and call goRemovePropertiesRequired
on AdditionalProperties.Schema.Properties (with nil checks), mirroring the
existing Items handling to ensure map values' nested required arrays are also
removed.
- Around line 1003-1021: The loop over k8sImports can yield non-deterministic
import ordering because maps iterate randomly; change the logic so after
building the needed map you collect the aliases from k8sImports into a slice,
sort that slice (e.g., with sort.Strings), then iterate the sorted aliases and
call astutil.AddNamedImport(fset, f, alias, path) only for aliases present in
needed; update references to k8sImports, needed, astutil.AddNamedImport, fset
and f accordingly so imports are added in a deterministic, sorted order.
- Around line 478-486: In writeGoCode, don't ignore the file close error:
replace the current explicit _ = f.Close() with a defer that captures Close's
error and surfaces it (e.g., defer func(){ if cerr := f.Close(); err == nil &&
cerr != nil { err = errors.Wrap(cerr, "failed to close go schema file") } }()),
ensuring you preserve the existing write error path (symbols to locate:
schemaFS.Create, f.WriteString, f.Close, writeGoCode). This ensures any
flush/close failure from the underlying FS is returned to the caller.
- Around line 1322-1349: The code in generateGVKGroupCode currently copies
schemas into groupSpec.Components.Schemas then immediately overwrites them by
copying openAPISpec.Components.Schemas (using maps.Copy), making the first copy
redundant; change the copy order so that maps.Copy(groupSpec.Components.Schemas,
openAPISpec.Components.Schemas) runs first and then maps.Copy(..., schemas) so
entries from the GVK-filtered schemas override the general spec, or remove the
first copy entirely if you prefer general-only merge; also add a defensive
length check on parts produced from strings.Split(gvkKey, "/") (or use
strings.Cut/extractGVKKey) to avoid panics when gvkKey is malformed, and add a
short comment explaining that we merge supporting types from the full OpenAPI
spec and allow GVK-specific schemas to take precedence.
- Around line 543-554: goFixName currently locks generateGoMutex (which is
documented as protecting the non-concurrency-safe codegen.Generate), causing
unintended coupling between callers of goFixName and long-running generate runs;
either document that generateGoMutex also protects codegen.SchemaNameToTypeName
and codegen.ToCamelCaseWithInitialisms, or split the lock so goFixName uses its
own mutex (e.g., goFixNameMutex) and leave generateGoMutex for codegen.Generate
only. Locate generateGoMutex, goFixName, and the helpers
codegen.SchemaNameToTypeName/codegen.ToCamelCaseWithInitialisms and implement
one of the two options: update the mutex comment to reflect the broader
contract, or replace the lock in goFixName with a dedicated mutex to avoid
blocking generate runs. Ensure tests still pass and add a brief comment
describing the new locking rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 916f0883-cbe9-45d8-8a55-38a092185600
⛔ Files ignored due to path filters (29)
cmd/crossplane/function/templates/go-templating/00-prelude.yaml.gotmplis excluded by none and included by nonecmd/crossplane/function/templates/go-templating/01-compose.yaml.gotmplis excluded by none and included by nonecmd/crossplane/function/templates/go.taris excluded by!**/*.tarand included by nonecmd/crossplane/function/templates/kcl/kcl.mod.lockis excluded by!**/*.lockand included by nonecmd/crossplane/function/templates/kcl/kcl.mod.tmplis excluded by none and included by nonecmd/crossplane/function/templates/kcl/main.kis excluded by none and included by nonecmd/crossplane/function/templates/python/function/__init__.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/__version__.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/fn.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/function/main.pyis excluded by none and included by nonecmd/crossplane/function/templates/python/pyproject.toml.tmplis excluded by none and included by nonego.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonegomod2nix.tomlis excluded by none and included by noneinternal/crd/testdata/claimable-xrd.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/crd/testdata/template.fn.crossplane.io_kclinputs.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/crd/testdata/unclaimable-xrd.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/project/functions/testdata/go-templating-function/resource1.yaml.gotmplis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/go-templating-function/resource2.yaml.tmplis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/kcl.modis excluded by!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/kcl.mod.lockis excluded by!**/*.lock,!**/testdata/**and included by noneinternal/project/functions/testdata/kcl-function/main.kis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/account_scaffold_composition.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/account_scaffold_definition.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/api__v1_openapi.jsonis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/api_openapi.jsonis excluded by!**/testdata/**and included by noneinternal/schemas/generator/testdata/azure_linux_function_app.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/generator/testdata/configuration_crossplane.yamlis excluded by!**/testdata/**and included by**/*.yamlinternal/schemas/runner/testdata/template.fn.crossplane.io_kclinputs.yamlis excluded by!**/testdata/**and included by**/*.yaml
📒 Files selected for processing (88)
cmd/crossplane/composition/composition.gocmd/crossplane/composition/generate.gocmd/crossplane/composition/generate_test.gocmd/crossplane/dependency/add.gocmd/crossplane/dependency/auth.gocmd/crossplane/dependency/cache.gocmd/crossplane/dependency/dependency.gocmd/crossplane/function/function.gocmd/crossplane/function/generate.gocmd/crossplane/function/generate_test.gocmd/crossplane/function/pipeline.gocmd/crossplane/function/templates/python/README.mdcmd/crossplane/main.gocmd/crossplane/project/build.gocmd/crossplane/project/init.gocmd/crossplane/project/project.gocmd/crossplane/project/push.gocmd/crossplane/project/run.gocmd/crossplane/project/stop.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/op/cmd_test.gocmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xr/cmd_test.gocmd/crossplane/xrd/generate.gocmd/crossplane/xrd/generate_test.gocmd/crossplane/xrd/xrd.gointernal/async/event.gointernal/crd/convert.gointernal/crd/convert_test.gointernal/crd/generator.gointernal/crd/generator_test.gointernal/dependency/cache_dir.gointernal/dependency/manager.gointernal/dependency/manager_test.gointernal/docker/docker.gointernal/filesystem/filesystem.gointernal/filesystem/filesystem_test.gointernal/git/git.gointernal/git/git_test.gointernal/kcl/import.gointernal/kcl/import_test.gointernal/project/build.gointernal/project/build_test.gointernal/project/certs/cert_generator.gointernal/project/certs/tls.gointernal/project/controlplane/controlplane.gointernal/project/functions/build.gointernal/project/functions/build_test.gointernal/project/functions/go.gointernal/project/functions/go_templating.gointernal/project/functions/kcl.gointernal/project/functions/python.gointernal/project/helm/helm.gointernal/project/helm/restclientgetter.gointernal/project/image.gointernal/project/install.gointernal/project/install_test.gointernal/project/projectfile/projectfile.gointernal/project/projectfile/projectfile_test.gointernal/project/push.gointernal/project/push_test.gointernal/project/render.gointernal/project/sort.gointernal/schemas/generator/go.gointernal/schemas/generator/go_test.gointernal/schemas/generator/interface.gointernal/schemas/generator/json.gointernal/schemas/generator/json_test.gointernal/schemas/generator/kcl.gointernal/schemas/generator/kcl_test.gointernal/schemas/generator/python.gointernal/schemas/generator/python_test.gointernal/schemas/manager/lock.gointernal/schemas/manager/manager.gointernal/schemas/manager/manager_test.gointernal/schemas/manager/source.gointernal/schemas/manager/source_test.gointernal/schemas/runner/run.gointernal/schemas/runner/run_test.gointernal/terminal/spinner.gointernal/xpkg/client.gointernal/xpkg/doc.gointernal/xpkg/fetcher.gointernal/xpkg/metadata.gointernal/xpkg/resolver.gointernal/xpkg/resolver_test.gointernal/xrd/infer.gointernal/xrd/infer_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crossplane/function/templates/python/README.md
🚧 Files skipped from review as they are similar to previous changes (63)
- cmd/crossplane/xrd/xrd.go
- internal/dependency/cache_dir.go
- cmd/crossplane/composition/composition.go
- internal/kcl/import_test.go
- cmd/crossplane/project/project.go
- internal/schemas/generator/interface.go
- cmd/crossplane/render/op/cmd_test.go
- cmd/crossplane/render/xr/cmd_test.go
- internal/crd/generator_test.go
- internal/project/projectfile/projectfile_test.go
- internal/async/event.go
- cmd/crossplane/dependency/auth.go
- internal/schemas/generator/go_test.go
- cmd/crossplane/dependency/cache.go
- cmd/crossplane/dependency/add.go
- internal/dependency/manager_test.go
- internal/project/build_test.go
- internal/project/sort.go
- internal/crd/generator.go
- internal/crd/convert_test.go
- cmd/crossplane/function/function.go
- internal/project/helm/restclientgetter.go
- cmd/crossplane/composition/generate_test.go
- cmd/crossplane/project/stop.go
- internal/project/push_test.go
- internal/filesystem/filesystem.go
- cmd/crossplane/project/init.go
- internal/docker/docker.go
- cmd/crossplane/main.go
- internal/project/helm/helm.go
- cmd/crossplane/xrd/generate.go
- cmd/crossplane/function/pipeline.go
- internal/schemas/generator/json_test.go
- internal/project/functions/kcl.go
- internal/project/functions/python.go
- cmd/crossplane/xrd/generate_test.go
- internal/schemas/generator/kcl_test.go
- internal/project/push.go
- internal/project/certs/tls.go
- internal/project/image.go
- cmd/crossplane/render/op/cmd.go
- internal/project/functions/go_templating.go
- internal/project/certs/cert_generator.go
- internal/project/render.go
- cmd/crossplane/function/generate.go
- cmd/crossplane/function/generate_test.go
- internal/project/functions/build_test.go
- internal/project/functions/build.go
- internal/project/projectfile/projectfile.go
- internal/schemas/generator/json.go
- cmd/crossplane/project/build.go
- internal/project/build.go
- internal/project/install_test.go
- internal/git/git.go
- internal/dependency/manager.go
- internal/git/git_test.go
- internal/project/install.go
- cmd/crossplane/project/run.go
- internal/crd/convert.go
- internal/filesystem/filesystem_test.go
- internal/schemas/generator/kcl.go
- internal/schemas/generator/python.go
- internal/project/controlplane/controlplane.go
| type generateCmd struct { | ||
| XRD string `arg:"" help:"Path to the CompositeResourceDefinition (XRD) file."` | ||
| Name string `help:"Name prefix for the composition." optional:""` | ||
| Plural string `help:"Custom plural for the CompositeTypeRef.Kind." optional:""` |
There was a problem hiding this comment.
Clarify --plural help text
Could we tweak this help text? Today it says plural is for CompositeTypeRef.Kind, but this flag actually affects derived plural/name/path behavior, not kind.
Proposed fix
- Plural string `help:"Custom plural for the CompositeTypeRef.Kind." optional:""`
+ Plural string `help:"Custom plural used for generated composition naming and output path." optional:""`As per coding guidelines "**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Plural string `help:"Custom plural for the CompositeTypeRef.Kind." optional:""` | |
| Plural string `help:"Custom plural used for generated composition naming and output path." optional:""` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/composition/generate.go` at line 53, Update the help text for
the Plural field to accurately describe what the flag controls: clarify that it
sets a custom plural/name/path for generated resources (affecting derived
plural/name/path behavior) rather than modifying the CompositeTypeRef.Kind;
locate the struct field named Plural (the line with `Plural string`) in
generate.go and replace the help string accordingly so the flag description
reflects derived plural/name/path behavior.
| func (c *generateCmd) AfterApply() error { | ||
| projFilePath, err := filepath.Abs(c.ProjectFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| projDirPath := filepath.Dir(projFilePath) | ||
| c.projFS = afero.NewBasePathFs(afero.NewOsFs(), projDirPath) | ||
|
|
||
| proj, err := projectfile.Parse(c.projFS, filepath.Base(c.ProjectFile)) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and check its structure
fd -t f "generate.go" cmd/Repository: crossplane/cli
Length of output: 164
🏁 Script executed:
# Get the file size to determine how to read it
wc -l cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 101
🏁 Script executed:
# Read the specific sections mentioned in the review
sed -n '1,100p' cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 3454
🏁 Script executed:
# Check for additional lines around 90-92 and 144-145
sed -n '85,150p' cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 2176
🏁 Script executed:
# Look for imports to verify error package usage
head -30 cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 888
🏁 Script executed:
# Verify errors.Wrapf is used elsewhere in the codebase
rg "errors\.Wrapf" cmd/ --max-count=5Repository: crossplane/cli
Length of output: 11424
🏁 Script executed:
# Look at the exact WriteFile line and surrounding context (lines 140-150)
sed -n '140,150p' cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 533
🏁 Script executed:
# Check for errors.Wrap usage patterns in Run() to confirm it's the established pattern
sed -n '100,145p' cmd/crossplane/composition/generate.goRepository: crossplane/cli
Length of output: 1499
Wrap raw errors with user-facing context in command flow
Looking good overall. The Run() method consistently wraps errors with helpful context for users, but the AfterApply() method and the WriteFile return on line 144 skip this context. Let's make error handling consistent throughout the command's lifecycle to help users understand what failed and what they were trying to do (project path resolution, project parsing, client init, output write).
The errors.Wrap and errors.Wrapf from the already-imported crossplane-runtime package are the right approach here—they're used consistently throughout the codebase and align with our CLI error messaging standards.
Suggested fix
projFilePath, err := filepath.Abs(c.ProjectFile)
if err != nil {
- return err
+ return errors.Wrapf(err, "failed to resolve project file path %q", c.ProjectFile)
}
@@
proj, err := projectfile.Parse(c.projFS, filepath.Base(c.ProjectFile))
if err != nil {
- return err
+ return errors.Wrapf(err, "failed to parse project file %q", c.ProjectFile)
}
@@
if err != nil {
- return err
+ return errors.Wrap(err, "failed to initialize package client")
}
@@
- return afero.WriteFile(c.apisFS, filePath, compYAML, 0o644)
+ if err := afero.WriteFile(c.apisFS, filePath, compYAML, 0o644); err != nil {
+ return errors.Wrapf(err, "failed to write generated Composition to %q", filePath)
+ }
+ return nil
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/composition/generate.go` around lines 65 - 76, The
AfterApply() method currently returns raw errors from filepath.Abs,
projectfile.Parse, and later file writes; update AfterApply() (and the WriteFile
error return in the generate flow) to wrap these errors using errors.Wrap or
errors.Wrapf from crossplane-runtime to provide user-facing context — e.g., wrap
the filepath.Abs error with "resolve project file path", wrap projectfile.Parse
error with "parse project file <ProjectFile>" (use filepath.Base(c.ProjectFile)
in the message), and wrap the WriteFile error with "write generated composition
to <output path>"; locate these fixes in the generateCmd.AfterApply method and
the file-write call in the same generate command implementation and replace
direct returns of err with errors.Wrap/errors.Wrapf including the descriptive
messages.
| group = xrd.Spec.Group | ||
| kind = xrd.Spec.Names.Kind | ||
| plural = xrd.Spec.Names.Plural | ||
| if c.Plural != "" { | ||
| plural = c.Plural | ||
| } |
There was a problem hiding this comment.
Validate plural before deriving names/paths
Thanks for shipping this command — quick edge case: if spec.names.plural is empty and --plural isn’t set, downstream defaults can produce invalid output (e.g., /composition.yaml) and invalid metadata.name fragments. Could we fail fast here with a clear message?
Proposed fix
group = xrd.Spec.Group
kind = xrd.Spec.Names.Kind
plural = xrd.Spec.Names.Plural
if c.Plural != "" {
plural = c.Plural
}
+ if plural == "" {
+ return "", "", "", "", errors.New("XRD spec.names.plural is required unless --plural is set")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| group = xrd.Spec.Group | |
| kind = xrd.Spec.Names.Kind | |
| plural = xrd.Spec.Names.Plural | |
| if c.Plural != "" { | |
| plural = c.Plural | |
| } | |
| group = xrd.Spec.Group | |
| kind = xrd.Spec.Names.Kind | |
| plural = xrd.Spec.Names.Plural | |
| if c.Plural != "" { | |
| plural = c.Plural | |
| } | |
| if plural == "" { | |
| return "", "", "", "", errors.New("XRD spec.names.plural is required unless --plural is set") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/composition/generate.go` around lines 223 - 228, Validate that
the plural name is non-empty before using it to derive names/paths: check
xrd.Spec.Names.Plural (and consider the override c.Plural) and if the resulting
plural is empty return a clear error (or fail fast) with a message like "plural
name is required: set spec.names.plural or --plural" instead of proceeding to
compute group/kind/plural and generating invalid paths/metadata; update the
logic around the plural assignment (where group = xrd.Spec.Group, kind =
xrd.Spec.Names.Kind, plural = xrd.Spec.Names.Plural and the c.Plural override)
to perform this validation and error reporting.
| type Cmd struct { | ||
| Add addCmd `cmd:"" help:"Add a dependency to the current project."` | ||
| UpdateCache updateCacheCmd `cmd:"" help:"Update the dependency cache for the current project."` | ||
| CleanCache cleanCacheCmd `cmd:"" help:"Clean the dependency cache."` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Please gate this new dependency command surface behind an experimental feature flag
Thanks for adding clear subcommand help text here. Since this wires in a brand-new DevEx command group, can we add (or reference) a feature gate at this registration point so experimental behavior is explicitly opt-in? If the gate is already enforced at a higher level, could you point to it so this path is clearly covered?
As per coding guidelines: "Implement feature flags for all new experimental features affecting apis/** or features that significantly affect behavior".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/dependency/dependency.go` around lines 21 - 25, The new Cmd
struct (fields Add, UpdateCache, CleanCache) must be registered only when the
experimental feature flag is enabled; update the command registration so the
dependency command group is created/returned conditionally (e.g., move creation
behind a NewDependencyCmd or GetDependencyCmd factory that checks a feature gate
function IsFeatureEnabled("dependencyCmd") or consults the existing feature flag
mechanism) and do not expose Cmd unconditionally; if the gate is already
enforced higher up, add a reference comment to that check from the Cmd
declaration and/or call to NewDependencyCmd so reviewers can see the protection.
| projFilePath, err := filepath.Abs(c.ProjectFile) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Add user-friendly context to the error message.
If filepath.Abs fails, the user won't know which path caused the issue or why. Consider wrapping with context about what the command was attempting to do.
🛡️ Proposed fix to add error context
projFilePath, err := filepath.Abs(c.ProjectFile)
if err != nil {
- return err
+ return errors.Wrapf(err, "failed to resolve project file path %q", c.ProjectFile)
}As per coding guidelines: "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projFilePath, err := filepath.Abs(c.ProjectFile) | |
| if err != nil { | |
| return err | |
| } | |
| projFilePath, err := filepath.Abs(c.ProjectFile) | |
| if err != nil { | |
| return errors.Wrapf(err, "failed to resolve project file path %q", c.ProjectFile) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/project/push.go` around lines 61 - 64, The call to
filepath.Abs in push.go that computes projFilePath does not wrap the error with
user-friendly context; update the error handling around
filepath.Abs(c.ProjectFile) (referencing projFilePath, filepath.Abs and
c.ProjectFile) to return a descriptive message such as "failed to resolve
project file path '<path>'" that includes the original path and suggests a next
step (e.g., verify the path or run from repo root); implement this by wrapping
the original err with fmt.Errorf or errors.Wrapf so the underlying error is
preserved while presenting clear, actionable context to the user.
| builder, err := build.NewGo(ctx, c.OSBasePath, | ||
| build.WithBaseImages(func(ctx context.Context, _ string) (name.Reference, build.Result, error) { | ||
| baseImage := b.baseImage | ||
| _, rewritten, err := b.configStore.RewritePath(ctx, baseImage) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "failed to rewrite base image") | ||
| } | ||
| if rewritten != "" { | ||
| baseImage = rewritten | ||
| } | ||
|
|
||
| ref, err := name.ParseReference(baseImage, name.StrictValidation) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain)) | ||
| return ref, img, err | ||
| }), | ||
| build.WithPlatforms(platforms...), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google/ko build.WithBaseImages callback signature and behavior when base image is not a multi-arch index
💡 Result:
The callback signature for build.WithBaseImages in google/ko is a GetBase function: type GetBase func(context.Context, string) (name.Reference, Result, error). This takes a context and importpath (string), returning a parsed image reference (name.Reference), the base image or index as a Result (v1.Image or v1.ImageIndex), and an error.[1][2][3] When the base image provided by the callback is not a multi-arch index (i.e., mediaType is application/vnd.oci.image.manifest.v1+json or application/vnd.docker.distribution.manifest.v2+json, not application/vnd.oci.image.index.v1+json or application/vnd.docker.distribution.manifest.list.v2+json), ko treats it as a single-architecture image. The platformMatcher (from --platform or defaults) selects a matching platform; if none matches, it likely fails or uses the base's platform. ko builds only for that single platform/architecture, producing a single-arch image, as confirmed in issues where single-arch bases limit multi-platform builds.[2][4][5][6]
Citations:
- 1: https://pkg.go.dev/github.com/google/ko@v0.9.3/pkg/build
- 2: https://github.com/ko-build/ko/blob/main/pkg/build/gobuild.go
- 3: https://pkg.go.dev/github.com/google/ko/pkg/build
- 4: ko only builds one platform when -L is specified ko-build/ko#1591
- 5: https://ko.build/features/multi-platform
- 6: --platform can be ignored by what the base image supports ko-build/ko#699
🏁 Script executed:
#!/bin/bash
# Check the actual code in the file to understand the context better
if git ls-files | grep -q "internal/project/functions/go.go"; then
echo "=== File content around lines 65-84 ==="
cat -n internal/project/functions/go.go | sed -n '55,95p'
fi
# Also search for any comments about base image selection or importpath handling
echo ""
echo "=== Searching for comments about baseImage or importpath ==="
rg -i "baseimage|importpath" internal/project/functions/go.go -B 2 -A 2Repository: crossplane/cli
Length of output: 2878
🌐 Web query:
remote.Index go-containerregistry single-arch image behavior
💡 Result:
In go-containerregistry, remote.Index resolves the reference to an OCI “index” (manifest list / image index) and then lets you pick a single platform from that index. When you request a single-arch image via platform selection and the requested platform isn’t present, the library falls back to amd64/linux. 1) What remote.Index does (even for single-arch) - remote.Index(ref,...) fetches a remote descriptor and then returns desc.ImageIndex, i.e., a v1.ImageIndex abstraction backed by the remote index manifest [1]. - remoteIndex’s logic for selecting an image by platform is implemented in remote/index.go, where it fetches the index, then chooses one child descriptor matching the requested platform, and “coerc[es] nested... into an Image” via Descriptor.Image [1]. 2) Platform filtering: how it matches - The platform matching function matches architecture and OS exactly. - OS version and variant must match if the required fields are provided. - OSFeatures and Features are treated as “subset” relationships (required feature sets must be subsets of the candidate platform’s feature sets) [1]. 3) Single-arch behavior when platform is missing - In remote/index.go, if the platform option is missing from the index, it “assume[s] it's amd64/linux” (fallback defaultPlatform) [1]. So, if you use remote.Index(...) and then select a platform (e.g., via ByPlatform) for an image/tag that is effectively “single-arch,” the behavior is: - If the index contains a matching platform descriptor, go-containerregistry returns that platform’s image. - If the requested platform doesn’t match any descriptor (or platform metadata can’t be used), it assumes amd64/linux [1].
Citations:
Wrap error returns from ParseReference and remote.Index with user-facing context
Thanks for the thoughtful questions! Based on verification, here's what I found:
-
remote.Indexwith single-arch images — Great news:remote.Indexhandles both multi-arch indices and single-arch images gracefully. It falls back toamd64/linuxif the requested platform isn't found in the index, so switching toremote.Imagewith fallback isn't needed here. -
Ignored
importpathparameter — Looks intentional by design. ThegoBuilderstruct maintains a singlebaseImagefor all functions in a project, and the callback deliberately ignores the importpath parameter (via_). A brief comment explaining this design would be helpful for future maintainers, though.
However, there's a pattern issue to address per Crossplane guidelines: Lines 78 and 81 return errors directly without user-facing context. Line 70 already shows the right approach with errors.Wrap(err, "failed to rewrite base image"). Consider wrapping the ParseReference and remote.Index errors similarly so users understand what went wrong and what to check next (e.g., invalid image reference format, network/auth issues, etc.).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/go.go` around lines 65 - 84, The callback passed
to build.NewGo returns raw errors from name.ParseReference and remote.Index;
wrap those errors with user-facing context using errors.Wrap or fmt.Errorf with
context to explain the operation and likely cause. Update the error returns in
the anonymous function (the base-images callback used by build.NewGo /
build.WithBaseImages) so that the ParseReference error is returned as wrapped
(e.g., errors.Wrap(err, "failed to parse base image reference") ) and the
remote.Index error is returned wrapped (e.g., errors.Wrap(err, "failed to fetch
base image index from registry") ), keeping the same return values (ref, img,
err) shape. Ensure the comment about intentionally ignoring the importpath (_)
remains or add a brief comment, and use the same error-wrapping style already
used for RewritePath to keep consistency.
| ref, err := name.ParseReference(baseImage, name.StrictValidation) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain)) | ||
| return ref, img, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Could we wrap these two errors for consistency with the rest of the function?
Nice work on the error handling everywhere else in this function — the wrapped messages are really helpful! These two return sites at Lines 78 and 81 surface the raw error to callers, which makes them stand out from the surrounding code and loses the "what was the CLI trying to do?" context for the user.
♻️ Suggested wrap
- ref, err := name.ParseReference(baseImage, name.StrictValidation)
- if err != nil {
- return nil, nil, err
- }
- img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain))
- return ref, img, err
+ ref, err := name.ParseReference(baseImage, name.StrictValidation)
+ if err != nil {
+ return nil, nil, errors.Wrapf(err, "failed to parse function base image reference %q", baseImage)
+ }
+ img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain))
+ return ref, img, errors.Wrapf(err, "failed to fetch function base image %q", baseImage)As per coding guidelines: "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users… include context about what the user was trying to do".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ref, err := name.ParseReference(baseImage, name.StrictValidation) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain)) | |
| return ref, img, err | |
| ref, err := name.ParseReference(baseImage, name.StrictValidation) | |
| if err != nil { | |
| return nil, nil, errors.Wrapf(err, "failed to parse function base image reference %q", baseImage) | |
| } | |
| img, err := remote.Index(ref, remote.WithTransport(b.transport), remote.WithAuthFromKeychain(authn.DefaultKeychain)) | |
| return ref, img, errors.Wrapf(err, "failed to fetch function base image %q", baseImage) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/go.go` around lines 76 - 81, Wrap the raw errors
returned from name.ParseReference and remote.Index with
crossplane-runtime/pkg/errors to match the rest of the function: when calling
name.ParseReference(ref, name.StrictValidation) wrap the error (e.g., using
errors.Wrapf) to include context like "parsing base image %s" referencing
baseImage/ name.ParseReference, and when calling remote.Index(ref,
remote.WithTransport(b.transport),
remote.WithAuthFromKeychain(authn.DefaultKeychain)) wrap that error to include
context like "fetching remote index for %s" referencing remote.Index, ref,
b.transport and authn.DefaultKeychain so callers get consistent, user-facing
messages.
| var group, kind, version string | ||
| switch pkg { | ||
| case k8sPkgMetaV1: | ||
| group = "meta.k8s.io" | ||
| kind = "meta" | ||
| version = "v1" | ||
| case k8sPkgAutoscalingV1: | ||
| group = k8sPkgNameAutoscaling | ||
| kind = k8sPkgNameAutoscaling | ||
| version = "v1" | ||
| } | ||
|
|
||
| // For K8s packages that reference meta.v1, we need to use the correct | ||
| // meta import path. The meta.v1 package uses goReferenceK8sTypes (core | ||
| // path) because self-references get stripped. Other packages like | ||
| // autoscaling use goReferenceK8sTypesForCRDs (non-core path) to | ||
| // reference the CRD meta.v1 package at | ||
| // dev.crossplane.io/models/io/k8s/meta/v1. | ||
| refMutator := goReferenceK8sTypes | ||
| if pkg != k8sPkgMetaV1 { | ||
| refMutator = goReferenceK8sTypesForCRDs | ||
| } | ||
|
|
||
| code, err := generateGo(pkgSpec, version, | ||
| goRenameTypes, | ||
| goRenameEnums, | ||
| goReplaceNumberWithInt, | ||
| goRemoveRequired, | ||
| refMutator, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // shorten the auto‑generated K8s type names | ||
| code, err = fixK8sTypeNames(code) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // remove the self‑import (e.g. meta/v1 importing itself) | ||
| code, err = removeSelfImports(code, pkg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := writeGoCode(schemaFS, group, kind, version, code); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing K8s package cases — runtime/core/intstr/resource will collide on the same file path.
Thanks for the careful comments explaining the meta-vs-autoscaling distinction! However, while tracing through, I noticed that goExtractK8sSchemas (lines 437-444) can emit entries for all six packages (k8sPkgMetaV1, k8sPkgRuntime, k8sPkgCoreV1, k8sPkgIntStr, k8sPkgResource, k8sPkgAutoscalingV1), but the switch pkg here only assigns group/kind/version for k8sPkgMetaV1 and k8sPkgAutoscalingV1. For the other four, group, kind, and version all stay as "".
When that reaches writeGoCode → goSchemaPath("", "", ""), the resulting path resolves to models/.go (a single dotfile), so any CRD-derived spec that pulls in e.g. runtime.RawExtension, IntOrString, or resource.Quantity will silently overwrite the previously written shared package into the same file. Compare with getK8sPackageInfo in GenerateFromOpenAPI (lines 1236-1251), which does cover all six packages — could we mirror that table here (or just call getK8sPackageInfo directly) so the CRD path is consistent?
Could you confirm whether CRD inputs are expected to ever reference these other K8s types? If yes, this looks like a release-blocker; if no, even a defensive default that errors out would make the constraint explicit.
🔧 Suggested approach
- // Determine the group, kind, and version from the package name
- var group, kind, version string
- switch pkg {
- case k8sPkgMetaV1:
- group = "meta.k8s.io"
- kind = "meta"
- version = "v1"
- case k8sPkgAutoscalingV1:
- group = k8sPkgNameAutoscaling
- kind = k8sPkgNameAutoscaling
- version = "v1"
- }
+ // Determine the group, kind, and version from the package name.
+ group, kind, version := getK8sPackageInfo(pkg)
+ if group == "" {
+ return nil, errors.Errorf("unsupported K8s package %q while generating CRD schemas", pkg)
+ }Note the group naming differs between the two paths (meta.k8s.io vs meta.core.k8s.io), so a small shared helper may be worth pulling out rather than a verbatim reuse — happy to sketch one if helpful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/go.go` around lines 197 - 246, The switch in
goExtractK8sSchemas only sets group/kind/version for k8sPkgMetaV1 and
k8sPkgAutoscalingV1, leaving other pkg values empty which causes goSchemaPath
(used by writeGoCode) to produce models/.go and overwrite files; fix by
populating the missing cases (k8sPkgRuntime, k8sPkgCoreV1, k8sPkgIntStr,
k8sPkgResource) with the same group/kind/version mapping used by
getK8sPackageInfo (or simply call getK8sPackageInfo(pkg) to retrieve the
canonical values) while preserving the existing refMutator logic, and add a
default branch that returns an explicit error if an unknown pkg is encountered
to avoid silent collisions.
| func goRenameTypes(s *spec3.OpenAPI) { | ||
| for name, schema := range s.Components.Schemas { | ||
| goName := goFixName(name) | ||
| if goName == "" { | ||
| delete(s.Components.Schemas, name) | ||
| } | ||
| goRenameSchemaType(goName, schema) | ||
| goRenamePropertyTypes(goName, schema.Properties) | ||
| } | ||
| } |
There was a problem hiding this comment.
delete is followed by side effects on the deleted entry — likely missing a continue.
In both goRenameTypes and goRenameEnums, when goFixName(name) returns "", we delete(s.Components.Schemas, name) but then immediately call goRenameSchemaType(goName, schema) / goRenameEnumValues(goName, schema) and walk properties using the empty goName. That seems to defeat the purpose of the delete: we'll still tag the (now-detached) schema with an empty x-go-type-name and recurse into its properties with an empty base name, which could cascade into oddly-named generated identifiers.
Is the intent here to skip the entry entirely once goName is empty? If so, a continue after delete would do the trick.
🔧 Proposed fix
for name, schema := range s.Components.Schemas {
goName := goFixName(name)
if goName == "" {
delete(s.Components.Schemas, name)
+ continue
}
goRenameSchemaType(goName, schema)
goRenamePropertyTypes(goName, schema.Properties)
} for name, schema := range s.Components.Schemas {
goName := goFixName(name)
if goName == "" {
delete(s.Components.Schemas, name)
+ continue
}
goRenameEnumValues(goName, schema)
goRenamePropertyEnums(goName, schema.Properties)
}Also applies to: 563-572
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/go.go` around lines 516 - 525, When
goFixName(name) returns an empty string in goRenameTypes (and likewise in
goRenameEnums), the code deletes the entry from s.Components.Schemas but
continues to call goRenameSchemaType/goRenameEnumValues and recurse into
properties with an empty goName; add a continue immediately after
delete(s.Components.Schemas, name) so the loop skips further processing for that
entry. Locate the map iteration in goRenameTypes and the analogous loop in
goRenameEnums, identify the use of goFixName(name), the delete call, and then
insert the continue to prevent calling goRenameSchemaType,
goRenamePropertyTypes, goRenameEnumValues or any further recursion when goName
== "".
| func fixMissingImports(code string) (string, error) { | ||
| // 1. Define the k8s imports you might need | ||
| k8sImports := map[string]string{ | ||
| "metacorev1": "dev.crossplane.io/models/io/k8s/core/meta/v1", | ||
| "metav1": "dev.crossplane.io/models/io/k8s/meta/v1", | ||
| "corev1": "dev.crossplane.io/models/io/k8s/core/v1", | ||
| "resourcev1": "dev.crossplane.io/models/io/k8s/resource/v1", | ||
| "runtimev1": "dev.crossplane.io/models/io/k8s/runtime/v1", | ||
| "intstrv1": "dev.crossplane.io/models/io/k8s/util/v1", | ||
| "autoscalingv1": "dev.crossplane.io/models/io/k8s/autoscaling/v1", | ||
| } | ||
|
|
||
| fset := token.NewFileSet() | ||
| f, err := parser.ParseFile(fset, "", code, parser.ParseComments) | ||
| if err != nil { | ||
| return "", fmt.Errorf("parsing failed: %w", err) | ||
| } | ||
|
|
||
| needed := map[string]bool{} | ||
| ast.Inspect(f, func(n ast.Node) bool { | ||
| if sel, ok := n.(*ast.SelectorExpr); ok { | ||
| if pkg, ok := sel.X.(*ast.Ident); ok { | ||
| if _, known := k8sImports[pkg.Name]; known { | ||
| needed[pkg.Name] = true | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| for alias, path := range k8sImports { | ||
| if !needed[alias] { | ||
| continue | ||
| } | ||
| // AddNamedImport will do nothing if the import (with that alias) is already present | ||
| astutil.AddNamedImport(fset, f, alias, path) | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| if err := printer.Fprint(&buf, fset, f); err != nil { | ||
| return "", fmt.Errorf("printing AST failed: %w", err) | ||
| } | ||
| return buf.String(), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Prefer crossplane-runtime/pkg/errors over fmt.Errorf for consistency.
Small consistency nit: fixMissingImports uses fmt.Errorf("parsing failed: %w", ...) and fmt.Errorf("printing AST failed: %w", ...), while the rest of this file uses errors.Wrap(...) from crossplane-runtime. Could we align this function with the surrounding style? It also gives end users slightly more context (e.g., "failed to parse generated Go code" rather than "parsing failed").
As per coding guidelines: "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users".
🔧 Proposed fix
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "", code, parser.ParseComments)
if err != nil {
- return "", fmt.Errorf("parsing failed: %w", err)
+ return "", errors.Wrap(err, "failed to parse generated Go code while fixing missing imports")
}
@@
var buf bytes.Buffer
if err := printer.Fprint(&buf, fset, f); err != nil {
- return "", fmt.Errorf("printing AST failed: %w", err)
+ return "", errors.Wrap(err, "failed to print Go AST after adding missing imports")
}(With this change, fmt no longer needs to be imported here.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func fixMissingImports(code string) (string, error) { | |
| // 1. Define the k8s imports you might need | |
| k8sImports := map[string]string{ | |
| "metacorev1": "dev.crossplane.io/models/io/k8s/core/meta/v1", | |
| "metav1": "dev.crossplane.io/models/io/k8s/meta/v1", | |
| "corev1": "dev.crossplane.io/models/io/k8s/core/v1", | |
| "resourcev1": "dev.crossplane.io/models/io/k8s/resource/v1", | |
| "runtimev1": "dev.crossplane.io/models/io/k8s/runtime/v1", | |
| "intstrv1": "dev.crossplane.io/models/io/k8s/util/v1", | |
| "autoscalingv1": "dev.crossplane.io/models/io/k8s/autoscaling/v1", | |
| } | |
| fset := token.NewFileSet() | |
| f, err := parser.ParseFile(fset, "", code, parser.ParseComments) | |
| if err != nil { | |
| return "", fmt.Errorf("parsing failed: %w", err) | |
| } | |
| needed := map[string]bool{} | |
| ast.Inspect(f, func(n ast.Node) bool { | |
| if sel, ok := n.(*ast.SelectorExpr); ok { | |
| if pkg, ok := sel.X.(*ast.Ident); ok { | |
| if _, known := k8sImports[pkg.Name]; known { | |
| needed[pkg.Name] = true | |
| } | |
| } | |
| } | |
| return true | |
| }) | |
| for alias, path := range k8sImports { | |
| if !needed[alias] { | |
| continue | |
| } | |
| // AddNamedImport will do nothing if the import (with that alias) is already present | |
| astutil.AddNamedImport(fset, f, alias, path) | |
| } | |
| var buf bytes.Buffer | |
| if err := printer.Fprint(&buf, fset, f); err != nil { | |
| return "", fmt.Errorf("printing AST failed: %w", err) | |
| } | |
| return buf.String(), nil | |
| } | |
| func fixMissingImports(code string) (string, error) { | |
| // 1. Define the k8s imports you might need | |
| k8sImports := map[string]string{ | |
| "metacorev1": "dev.crossplane.io/models/io/k8s/core/meta/v1", | |
| "metav1": "dev.crossplane.io/models/io/k8s/meta/v1", | |
| "corev1": "dev.crossplane.io/models/io/k8s/core/v1", | |
| "resourcev1": "dev.crossplane.io/models/io/k8s/resource/v1", | |
| "runtimev1": "dev.crossplane.io/models/io/k8s/runtime/v1", | |
| "intstrv1": "dev.crossplane.io/models/io/k8s/util/v1", | |
| "autoscalingv1": "dev.crossplane.io/models/io/k8s/autoscaling/v1", | |
| } | |
| fset := token.NewFileSet() | |
| f, err := parser.ParseFile(fset, "", code, parser.ParseComments) | |
| if err != nil { | |
| return "", errors.Wrap(err, "failed to parse generated Go code while fixing missing imports") | |
| } | |
| needed := map[string]bool{} | |
| ast.Inspect(f, func(n ast.Node) bool { | |
| if sel, ok := n.(*ast.SelectorExpr); ok { | |
| if pkg, ok := sel.X.(*ast.Ident); ok { | |
| if _, known := k8sImports[pkg.Name]; known { | |
| needed[pkg.Name] = true | |
| } | |
| } | |
| } | |
| return true | |
| }) | |
| for alias, path := range k8sImports { | |
| if !needed[alias] { | |
| continue | |
| } | |
| // AddNamedImport will do nothing if the import (with that alias) is already present | |
| astutil.AddNamedImport(fset, f, alias, path) | |
| } | |
| var buf bytes.Buffer | |
| if err := printer.Fprint(&buf, fset, f); err != nil { | |
| return "", errors.Wrap(err, "failed to print Go AST after adding missing imports") | |
| } | |
| return buf.String(), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/go.go` around lines 985 - 1028, The function
fixMissingImports currently uses fmt.Errorf for errors; replace those with
errors.Wrapf from crossplane-runtime/pkg/errors and update the messages to be
user-friendly (e.g., "failed to parse generated Go code: %v" and "failed to
print generated Go AST: %v") so callers get meaningful context; update the two
error returns inside fixMissingImports to use errors.Wrapf(err, "<message>") and
remove the now-unused fmt import.
3 participants
Description of your changes
This PR introduces the initial set of DevEx tools based around the concept of control plane Projects. Specifically, it introduces tools for:
crossplane composition renderandcrossplane operation render.Fixes crossplane/crossplane#6840
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.