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() |
There was a problem hiding this comment.
Updated to reject relative paths that leave the path prefix.
adamwg
force-pushed
the
awg/devex
branch
2 times, most recently
from
3d3d93c to
23c2b56
Compare
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (29)
📒 Files selected for processing (88)
📝 WalkthroughWalkthroughAdds Project API and types, parsing/validation, dependency and schema managers, multi-language schema generators, function builders, project build/push/run flows, local Kind dev control plane, render/composition/XRD commands, CLI wiring, utilities, and tests. ChangesProject developer tooling and CLI
Estimated code review effort Possibly related issues
Thank you — would you like this large checkpoint split into focused review passes (API & projectfile, dependency manager, schema generators, builders & packaging, control-plane)? |
![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
| 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.
There was a problem hiding this comment.
Updated the help to make it clearer that the project file is optional, and the functions file can be omitted when using render in a project.
| 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 ?
There was a problem hiding this comment.
In up we've added AdditionalMetadata, which can hold arbitrary k/v pairs that are specifically used by the Upbound marketplace (and must be prefixed with meta.upbound.io/). I've left those out here for now since they're pretty Upbound-specific and the k/v pairs get copied directly into the resulting configuration package's metadata.annotations so it's possible to fill them in just by writing them directly in the project's metadata.annotations. We can always add them in the future if desired.
| // 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. |
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
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>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
internal/kcl/import_test.go (1)
28-31: ⚡ Quick winAlign test table shape with repo convention (
args/want+reason).Thanks for adding strong coverage here. Could we reshape each case to include
args/wantand a shortreasonfield to match the test guideline pattern and make failure intent clearer?Proposed refactor
- tests := map[string]struct { - paths []string - wantImports map[string]string - }{ + tests := map[string]struct { + reason string + args struct { + paths []string + } + want map[string]string + }{ "BasicPath": { - paths: []string{"kcl/io.upbound.aws.ec2.v1beta1"}, - wantImports: map[string]string{"ec2v1beta1": "models.io.upbound.aws.ec2.v1beta1"}, + reason: "build alias from last two segments for a standard dotted path", + args: struct { + paths []string + }{paths: []string{"kcl/io.upbound.aws.ec2.v1beta1"}}, + want: map[string]string{"ec2v1beta1": "models.io.upbound.aws.ec2.v1beta1"}, },- gotImports := FormatKclImportPaths(tc.paths) - if diff := cmp.Diff(tc.wantImports, gotImports); diff != "" { + gotImports := FormatKclImportPaths(tc.args.paths) + if diff := cmp.Diff(tc.want, gotImports); diff != "" { t.Errorf("importPath mismatch (-want +got):\n%s", diff) }As per coding guidelines: "
**/*_test.go: Enforce table-driven test structure ... args/want pattern ... proper test case naming and reason fields."Also applies to: 32-63
🤖 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_test.go` around lines 28 - 31, The test table `tests` in import_test.go should follow the repo convention by replacing the current shape (paths []string, wantImports map[string]string) with the args/want/reason pattern; change each case to: struct{ args struct{ paths []string }; want map[string]string; reason string } and move existing `paths` into `args.paths`, `wantImports` into `want`, and add a short `reason` for each case, then update all references in the test loop that read `paths`/`wantImports` to use `tc.args.paths` and `tc.want` respectively (look for the `tests` variable and usages in the test function to modify).internal/project/functions/build_test.go (1)
121-127: ⚡ Quick winUse cmp-based error assertions instead of exact string matching.
Would you be open to switching this to an
args/want-style table entry and asserting errors withcmp.Diff(..., cmpopts.EquateErrors())? It makes these tests less brittle as messages evolve.As per coding guidelines: "
**/*_test.go: ... use cmp.Diff with cmpopts.EquateErrors() for error testing."🤖 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/build_test.go` around lines 121 - 127, Replace the brittle exact string comparisons in the test's error assertions by adding an args/want-style error entry to the test case table (e.g., extend each tc with a wantErr error or nil) and then assert using cmp.Diff(err, tc.wantErr, cmpopts.EquateErrors()) (instead of comparing err.Error() to a literal). Locate the block using tc.expectError/err in build_test.go, remove the direct string match ("no suitable builder found") and use cmp.Diff with cmpopts.EquateErrors() to report mismatches; ensure imports include "github.com/google/go-cmp/cmp" and "github.com/google/go-cmp/cmp/cmpopts".internal/schemas/generator/python.go (6)
400-400: 💤 Low valueCurious about the
depthcomputation against the full path.
depth := strings.Count(filePath, string(os.PathSeparator))counts separators in whatever absolute-ish path the caller hands in, which today includes thesorted/...(ormodels/models/...) prefix. That means the number of leading dots produced byadjustLeadingDotsdepends on where in the in‑memory FS the file happens to live, not on its position within the generated Python package. It currently works because everything funnels throughpostTransformCRDwith a stabletargetDir, but it's a sharp edge if either entry point changes.Would it be worth computing depth relative to the package root (e.g., from
pythonAdoptModelsStructure/pythonPackageRoot) so the import math is anchored to the package layout rather than the FS layout? Happy to be told this is fine as-is if I'm missing context.🤖 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` at line 400, The current depth calculation uses strings.Count(filePath, string(os.PathSeparator)) which counts separators in the full filesystem path and can vary with in-memory FS layout; change depth to be measured relative to the Python package root (e.g., pythonPackageRoot / value from pythonAdoptModelsStructure or postTransformCRD) by computing the path relative to that package root (use filepath.Rel or trim the package root prefix) and then count separators in the relative path before calling adjustLeadingDots; update the code that sets depth (the variable named depth near filePath) to derive the relative path first and fall back to the original behavior only if computing the relative path fails.
308-308: ⚡ Quick win
os.ModePerm(0o777) is awfully generous for.pysource and__init__.pyfiles.Small nit, but every write here uses
os.ModePermwhich is0o777— world‑writable + executable for files that are just Python sources.finalizePythonSchemasalready uses0o644for the seeded__init__.py/pyproject.toml, so we're a little inconsistent. Could we standardize on0o644for these writes? Same applies to the other call sites flagged in the line range.Also applies to: 376-376, 385-385, 417-417, 555-555, 667-667, 673-673, 740-740
🤖 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` at line 308, The file internal/schemas/generator/python.go uses os.ModePerm (0o777) for writing .py and __init__.py files; update those WriteFile calls to use a restrictive file mode (0644) to match finalizePythonSchemas and avoid world-writable/executable source files—replace os.ModePerm with 0644 in the WriteFile invocations found in this file (including the call at the shown site and the other flagged call sites) so Python source files are created with correct permissions.
50-50: 💤 Low valueHardcoded code-generator image / tag — any plan for overrides?
pythonImage = "docker.io/koxudaxi/datamodel-code-generator:0.31.2"is pinned here, which is great for reproducibility, but it's the only knob users have if they're behind an internal registry / mirror or want to bump versions ahead of a CLI release. Have you considered surfacing this via an env var or a flag on the dependency/schema commands (similar to how other Crossplane CLI tools allow overriding image references)? Not blocking — just flagging in case it's already on the roadmap.🤖 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` at line 50, The pythonImage constant is hardcoded which prevents overrides; make it configurable by reading an environment variable (e.g., DATAGEN_PYTHON_IMAGE) or by wiring a CLI flag into the same code path that uses pythonImage so callers can override it—keep the existing value "docker.io/koxudaxi/datamodel-code-generator:0.31.2" as the default when the env/flag is empty; update references to pythonImage (the pythonImage variable and any functions that consume it) to use the resolved value so users behind internal registries or wanting newer tags can override it.
602-602: 💤 Low valueUnused named return
openapiFolder.
normalizeAndFilterPathdeclaresopenapiFolderas a named return, but every caller discards it with_. If it's not part of the intended API, we could drop it from the signature to keep the surface area honest; if it is, we should probably use it (or document why it's exposed). Tiny thing — feel free to ignore.🤖 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` at line 602, The function normalizeAndFilterPath currently declares an unused named return openapiFolder; remove this from the signature and return only (normalizedParts []string, include bool) to shrink the API surface, then update all callers (which currently do _, normalizedParts, include := normalizeAndFilterPath(...)) to capture the two returned values (e.g. normalizedParts, include := normalizeAndFilterPath(...)); alternatively, if openapiFolder was meant to be used, populate and return it from normalizeAndFilterPath and update callers to use it—pick one approach and make caller sites and the function signature consistent.
267-268: 💤 Low value
--target-python-version 3.12vsrequires-python = ">=3.11,<3.14".The generator is told to target 3.12, but the
pyproject.tomlwe emit declares support for 3.11–3.13. In practice 3.12 output is usually backward-compatible to 3.11 and forward-compatible to 3.13, but if datamodel-code-generator ever starts emitting 3.12-only syntax (e.g., newtypestatement uses), 3.11 consumers would break. Would it make sense to either (a) target 3.11 here to match the floor ofrequires-python, or (b) tightenrequires-pythonto>=3.12? Curious which direction matches the intended user base.🤖 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 267 - 268, The generator currently passes "--target-python-version", "3.12" while the emitted pyproject.toml sets requires-python = ">=3.11,<3.14", creating a mismatch; update the generator to target "3.11" (replace the "--target-python-version","3.12" literal in internal/schemas/generator/python.go) so the datamodel-code-generator invocation aligns with the pyproject's minimum Python version, or alternatively tighten the emitted requires-python to ">=3.12" if you intend to require 3.12+—pick one consistent change and apply it to the corresponding symbol(s) ("--target-python-version" value or the generated requires-python string) so both agree.
46-49: 💤 Low value
pythonModelsFolderandpythonPackageRootare both"models"— a brief comment would help future spelunkers.It took me a second to realize that
filepath.Join(pythonModelsFolder, pythonPackageRoot)deliberately producesmodels/models(outer dir holdspyproject.toml, inner dir is the importable Python package referenced bypackages = ["models"]in the pyproject). Could we add a short comment on these constants explaining the two-layer layout? Would save the next reader a trip throughfinalizePythonSchemas.🤖 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 46 - 49, Add a short clarifying comment next to the constants pythonModelsFolder and pythonPackageRoot explaining that the project uses a two-layer layout: the outer folder (pythonModelsFolder) contains pyproject.toml and build metadata while the inner folder (pythonPackageRoot) is the actual importable Python package (hence filepath.Join(pythonModelsFolder, pythonPackageRoot) producing models/models), and mention that finalizePythonSchemas relies on this structure; update the comment near pythonModelsFolder, pythonPackageRoot (and optionally pythonGeneratedFolder) to briefly state this intent so future readers understand the models/models layout without tracing finalizePythonSchemas.
🤖 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/project/push.go`:
- Line 84: The code unguardedly asserts http.DefaultTransport.(*http.Transport)
into t which can panic if DefaultTransport is replaced; change it to a safe type
assertion (e.g., transport, ok := http.DefaultTransport.(*http.Transport)) and
handle the !ok case by returning a clear CLI error (or logging and exiting)
instead of allowing a panic; update the instance in
cmd/crossplane/project/push.go (the variable t) and make the same defensive
change at the matching site in cmd/crossplane/xpkg/push.go (around the transport
clone there) so both places validate the type before cloning.
In `@internal/project/functions/build_test.go`:
- Line 146: Tests that call NewStaticImageConfigStore (e.g., TestIdentify and
the two other tests flagged) are being run with t.Parallel(), causing races
around AddToScheme/static store initialization; fix by removing t.Parallel()
from those tests so they run serially, or alternatively make
NewStaticImageConfigStore/AddToScheme safe for concurrent use (e.g., protect
static initialization with sync.Once or a package mutex inside
NewStaticImageConfigStore) and update the tests accordingly.
- Around line 295-330: The reader returned by layer.Uncompressed() (rc) must be
closed and the return value from afero.Walk must be checked; after rc, call
rc.Close() via defer immediately after successful Uncompressed() (i.e., right
after rc, err := layer.Uncompressed()) to ensure the tar reader is closed, and
capture the error returned by afero.Walk(...) (replace the current ignored
return) and call t.Fatal(err) if non-nil so traversal failures are not
swallowed; reference layer.Uncompressed, rc, and afero.Walk in the change.
In `@internal/schemas/generator/python.go`:
- Around line 719-741: transformMetaImportsInFile currently drops scanner.Err()
and loses a trailing newline; extract a helper transformLines(fs, filePath,
transformFunc) that encapsulates bufio.Scanner buffer size/config, checks and
returns scanner.Err(), preserves the file's trailing newline when writing, and
returns an error; update transformMetaImportsInFile and adjustImportsInFile to
call transformLines passing a function that applies transformMetaImport and
adjustRelativeImportsForCoreMeta (or the existing import-adjust logic) so both
functions reuse consistent scanning, error handling, and newline-preservation
behavior.
- Around line 209-213: The current code silently falls back to the original
bytes when processedDoc.MarshalJSON() fails, losing the injected apiVersion/kind
from processOpenAPIContent without notifying callers; update the error handling
in the block around processedDoc.MarshalJSON() so that MarshalJSON errors are
not ignored—either return/propagate the error up to the caller (so callers know
enrichment failed) or at minimum log a warning including the error and the
filename/identifier before deciding to use bs; modify the logic in the function
that calls processOpenAPIContent (and the processedContent/bs handling) to
propagate the error path (or emit the logger call) rather than silently
continuing.
- Around line 399-418: The scanner-based rewrite in adjustImportsInFile (and the
similar transformMetaImportsInFile) must handle scanner errors and preserve the
original file's trailing newline: after scanning, check scanner.Err() and return
a wrapped error if non-nil (this prevents silent truncation on read errors or
bufio.MaxScanTokenSize hits), and when writing back, detect whether the original
fileContent ended with a newline and, if so, append a terminating "\n" to the
joined modifiedContent; alternatively replace the whole file body using a single
regexp on string(fileContent) (safer for long lines) instead of line-by-line
scanning — update functions adjustImportsInFile and transformMetaImportsInFile
accordingly and extract any shared logic into a small helper to avoid
duplication.
---
Nitpick comments:
In `@internal/kcl/import_test.go`:
- Around line 28-31: The test table `tests` in import_test.go should follow the
repo convention by replacing the current shape (paths []string, wantImports
map[string]string) with the args/want/reason pattern; change each case to:
struct{ args struct{ paths []string }; want map[string]string; reason string }
and move existing `paths` into `args.paths`, `wantImports` into `want`, and add
a short `reason` for each case, then update all references in the test loop that
read `paths`/`wantImports` to use `tc.args.paths` and `tc.want` respectively
(look for the `tests` variable and usages in the test function to modify).
In `@internal/project/functions/build_test.go`:
- Around line 121-127: Replace the brittle exact string comparisons in the
test's error assertions by adding an args/want-style error entry to the test
case table (e.g., extend each tc with a wantErr error or nil) and then assert
using cmp.Diff(err, tc.wantErr, cmpopts.EquateErrors()) (instead of comparing
err.Error() to a literal). Locate the block using tc.expectError/err in
build_test.go, remove the direct string match ("no suitable builder found") and
use cmp.Diff with cmpopts.EquateErrors() to report mismatches; ensure imports
include "github.com/google/go-cmp/cmp" and
"github.com/google/go-cmp/cmp/cmpopts".
In `@internal/schemas/generator/python.go`:
- Line 400: The current depth calculation uses strings.Count(filePath,
string(os.PathSeparator)) which counts separators in the full filesystem path
and can vary with in-memory FS layout; change depth to be measured relative to
the Python package root (e.g., pythonPackageRoot / value from
pythonAdoptModelsStructure or postTransformCRD) by computing the path relative
to that package root (use filepath.Rel or trim the package root prefix) and then
count separators in the relative path before calling adjustLeadingDots; update
the code that sets depth (the variable named depth near filePath) to derive the
relative path first and fall back to the original behavior only if computing the
relative path fails.
- Line 308: The file internal/schemas/generator/python.go uses os.ModePerm
(0o777) for writing .py and __init__.py files; update those WriteFile calls to
use a restrictive file mode (0644) to match finalizePythonSchemas and avoid
world-writable/executable source files—replace os.ModePerm with 0644 in the
WriteFile invocations found in this file (including the call at the shown site
and the other flagged call sites) so Python source files are created with
correct permissions.
- Line 50: The pythonImage constant is hardcoded which prevents overrides; make
it configurable by reading an environment variable (e.g., DATAGEN_PYTHON_IMAGE)
or by wiring a CLI flag into the same code path that uses pythonImage so callers
can override it—keep the existing value
"docker.io/koxudaxi/datamodel-code-generator:0.31.2" as the default when the
env/flag is empty; update references to pythonImage (the pythonImage variable
and any functions that consume it) to use the resolved value so users behind
internal registries or wanting newer tags can override it.
- Line 602: The function normalizeAndFilterPath currently declares an unused
named return openapiFolder; remove this from the signature and return only
(normalizedParts []string, include bool) to shrink the API surface, then update
all callers (which currently do _, normalizedParts, include :=
normalizeAndFilterPath(...)) to capture the two returned values (e.g.
normalizedParts, include := normalizeAndFilterPath(...)); alternatively, if
openapiFolder was meant to be used, populate and return it from
normalizeAndFilterPath and update callers to use it—pick one approach and make
caller sites and the function signature consistent.
- Around line 267-268: The generator currently passes "--target-python-version",
"3.12" while the emitted pyproject.toml sets requires-python = ">=3.11,<3.14",
creating a mismatch; update the generator to target "3.11" (replace the
"--target-python-version","3.12" literal in
internal/schemas/generator/python.go) so the datamodel-code-generator invocation
aligns with the pyproject's minimum Python version, or alternatively tighten the
emitted requires-python to ">=3.12" if you intend to require 3.12+—pick one
consistent change and apply it to the corresponding symbol(s)
("--target-python-version" value or the generated requires-python string) so
both agree.
- Around line 46-49: Add a short clarifying comment next to the constants
pythonModelsFolder and pythonPackageRoot explaining that the project uses a
two-layer layout: the outer folder (pythonModelsFolder) contains pyproject.toml
and build metadata while the inner folder (pythonPackageRoot) is the actual
importable Python package (hence filepath.Join(pythonModelsFolder,
pythonPackageRoot) producing models/models), and mention that
finalizePythonSchemas relies on this structure; update the comment near
pythonModelsFolder, pythonPackageRoot (and optionally pythonGeneratedFolder) to
briefly state this intent so future readers understand the models/models layout
without tracing finalizePythonSchemas.
🪄 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: 5cb6ef39-0822-43ea-8456-fa21a0a6a271
⛔ 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
✅ 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 (69)
- cmd/crossplane/function/function.go
- generate.go
- cmd/crossplane/xrd/xrd.go
- internal/schemas/generator/kcl_test.go
- internal/crd/generator_test.go
- internal/dependency/cache_dir.go
- cmd/crossplane/dependency/auth.go
- internal/project/sort.go
- internal/schemas/generator/go_test.go
- cmd/crossplane/project/project.go
- internal/project/build_test.go
- apis/dev/v1alpha1/doc.go
- cmd/crossplane/composition/composition.go
- cmd/crossplane/main.go
- internal/schemas/generator/json_test.go
- cmd/crossplane/function/pipeline.go
- internal/async/event.go
- cmd/crossplane/function/generate_test.go
- internal/project/render.go
- internal/docker/docker.go
- internal/project/projectfile/projectfile.go
- internal/project/functions/build.go
- cmd/crossplane/dependency/dependency.go
- cmd/crossplane/render/op/cmd_test.go
- internal/crd/convert_test.go
- internal/crd/convert.go
- internal/project/functions/go.go
- cmd/crossplane/dependency/cache.go
- internal/project/certs/tls.go
- internal/project/push_test.go
- internal/schemas/generator/json.go
- apis/dev/v1alpha1/validate.go
- internal/project/projectfile/projectfile_test.go
- apis/dev/v1alpha1/validate_test.go
- internal/project/functions/kcl.go
- internal/project/functions/go_templating.go
- cmd/crossplane/project/stop.go
- internal/git/git.go
- apis/dev/v1alpha1/defaults.go
- cmd/crossplane/xrd/generate_test.go
- cmd/crossplane/dependency/add.go
- apis/dev/v1alpha1/project_types.go
- internal/project/functions/python.go
- cmd/crossplane/function/generate.go
- internal/project/install_test.go
- internal/project/certs/cert_generator.go
- internal/dependency/manager_test.go
- internal/project/image.go
- internal/project/helm/restclientgetter.go
- cmd/crossplane/composition/generate.go
- cmd/crossplane/render/op/cmd.go
- cmd/crossplane/xrd/generate.go
- internal/project/push.go
- internal/schemas/generator/kcl.go
- internal/schemas/generator/interface.go
- internal/crd/generator.go
- cmd/crossplane/project/build.go
- internal/git/git_test.go
- internal/filesystem/filesystem.go
- internal/project/controlplane/controlplane.go
- internal/project/build.go
- internal/project/helm/helm.go
- internal/dependency/manager.go
- internal/project/install.go
- internal/filesystem/filesystem_test.go
- internal/schemas/generator/go.go
- cmd/crossplane/composition/generate_test.go
- cmd/crossplane/render/xr/cmd.go
- cmd/crossplane/project/run.go
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
cmd/crossplane/project/push.go (4)
61-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd user-friendly context to the error message.
If
filepath.Absfails, 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."
🤖 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(c.ProjectFile) returns a raw error with no user-friendly context; update the error handling around that call (the projFilePath, err := filepath.Abs(c.ProjectFile) block) to wrap the error with a clear message that includes the attempted path (c.ProjectFile) and a brief next step, e.g., using fmt.Errorf or errors.Wrapf to produce: "failed to resolve absolute path for project file '<path>': <err>; please check the path and permissions." Ensure the wrapped error is returned in place of the raw err.
99-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the error message more user-friendly.
The error "failed to parse repository" uses technical jargon and doesn't guide the user. When a user provides an invalid
--repositoryvalue, they need to know what format is expected.💬 Proposed fix for clearer error message
if c.Repository != "" { ref, err := name.NewRepository(c.Repository) if err != nil { - return errors.Wrap(err, "failed to parse repository") + return errors.Wrapf(err, "invalid repository %q: must be a valid OCI repository reference (e.g., ghcr.io/crossplane/example)", c.Repository) } c.proj.Spec.Repository = ref.String() }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. 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/push.go` around lines 99 - 105, The error returned when parsing c.Repository via name.NewRepository is too technical; update the error handling in the block that calls name.NewRepository (where c.Repository is parsed and assigned to c.proj.Spec.Repository) to return a user-friendly message that explains the expected repository format (e.g., "registry.example.com/my-repo[:tag]" or "host/namespace/repo"), includes the invalid value provided, and suggests how to fix it (for example "provide a valid container registry repository like 'ghcr.io/org/repo'"). Keep the original wrapped error for debugging but present the clearer message to the end user.
148-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImprove error messages for better user experience.
The error messages in this function use technical jargon ("manifest", "parse") and don't help users understand what went wrong or what to check. When a user encounters these errors, they need actionable guidance.
💬 Proposed fix for clearer error messages
mfst, err := tarball.LoadManifest(opener) if err != nil { - return nil, errors.Wrap(err, "failed to read package file manifest") + return nil, errors.Wrapf(err, "failed to read package file %q: ensure it is a valid .xpkg file created by 'crossplane project build'", c.PackageFile) } imgMap := make(project.ImageTagMap) for _, desc := range mfst { if len(desc.RepoTags) == 0 { // Ignore images with no tags; we shouldn't find these in xpkg // files, but best not to panic if it happens. continue } tag, err := name.NewTag(desc.RepoTags[0]) if err != nil { - return nil, errors.Wrapf(err, "failed to parse image tag %q", desc.RepoTags[0]) + return nil, errors.Wrapf(err, "invalid image tag %q in package file: the package may be corrupted", desc.RepoTags[0]) } image, err := tarball.Image(opener, &tag) if err != nil { - return nil, errors.Wrapf(err, "failed to load image %q from package", tag) + return nil, errors.Wrapf(err, "failed to load image %q from package file: the package may be corrupted or incomplete", tag) } imgMap[tag] = image }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. 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/push.go` around lines 148 - 177, The error messages in loadPackages should be rewritten to be user-facing and actionable: when tarball.LoadManifest(opener) fails (currently wrapped with "failed to read package file manifest"), change it to a clear message like "cannot open package file '<PackageFile>' — ensure the file exists and is a valid package archive" and include the original error; when name.NewTag(desc.RepoTags[0]) fails (currently "failed to parse image tag"), replace with "invalid image tag '<tag>' in package — check image name format (e.g. repo:tag) in the package" and include the underlying error; when tarball.Image(opener, &tag) fails (currently "failed to load image ... from package"), replace with "cannot load image '<tag>' from package — package may be corrupted or missing image layers" and include the original error; update the errors.Wrapf calls around these sites (loadPackages, tarball.LoadManifest, name.NewTag, tarball.Image) to use these friendlier messages while still wrapping the original error for diagnostics.
84-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the
http.DefaultTransporttype assertion to prevent a panic if the transport is ever replaced.While
http.DefaultTransportis always*http.Transportin Go's standard library, a defensive guard here is good practice—especially since this is public CLI code. Note that the same pattern appears incmd/crossplane/xpkg/push.go:126and should be addressed consistently for better resilience.🛡️ Suggested fix
- t := http.DefaultTransport.(*http.Transport).Clone() //nolint:forcetypeassert // http.DefaultTransport is always *http.Transport + baseTransport, ok := http.DefaultTransport.(*http.Transport) + if !ok { + return errors.New("failed to prepare HTTP transport: unexpected default transport type") + } + t := baseTransport.Clone()🤖 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` at line 84, The code unguardedly type-asserts http.DefaultTransport to *http.Transport when creating t (t := http.DefaultTransport.(*http.Transport).Clone()), which can panic if DefaultTransport is replaced; update this to perform a safe type assertion (tr, ok := http.DefaultTransport.(*http.Transport)) and if ok set t = tr.Clone() else create a sensible fallback transport (e.g., t = http.DefaultTransport or &http.Transport{} configured as needed) to avoid panics; apply the same guarded pattern to the analogous occurrence in xpkg/push.go (the type assertion at push setup) so both places safely handle non-*http.Transport DefaultTransport values.internal/docker/docker.go (1)
509-512:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winZip-Slip guard appears to be a no-op —
filepath.SplitListsplits PATH-style lists, not path components.Thanks for adding an explicit traversal check here! Could you double-check the choice of
filepath.SplitListthough? It splits strings using the OS PATH list separator (:on Unix,;on Windows), not the path separator — so for a malicious tar entry like../etc/passwd, afterfilepath.Cleanyou get"../etc/passwd", andfilepath.SplitList("../etc/passwd")returns a single element["../etc/passwd"], which doesn't contain a bare"..". The guard then passes and the file is happily written outside the intended destination. This also lines up with the CodeQL "Zip Slip" finding flagged on line 497.A few thoughts on how to make this watertight — happy to iterate on the approach you prefer:
- Use
filepath.IsLocal(Go 1.20+) on the cleaned, slash-form entry name, which explicitly rejects paths that escape, are absolute, or reference reserved names.- Or resolve the intended absolute destination and confirm the final cleaned path stays within it (e.g.
strings.HasPrefix(absChild, absRoot+string(os.PathSeparator))).Also note that tar entries from Docker use
/as the separator regardless of host OS, so mixingfilepath.Clean/filepath.Basewith tar-relative names can subtly misbehave on Windows; using thepathpackage for the tar-side normalization andfilepathonly when writing toafero.Fsis usually safer.🛡️ One possible fix using
pathfor tar-side handling +filepath.IsLocal- cleanedPath := filepath.Clean(strings.TrimPrefix(header.Name, filepath.Base(path)+"/")) - if slices.Contains(filepath.SplitList(cleanedPath), "..") { - return errors.Errorf("file %s lives outside path %s - refusing to copy", cleanedPath, path) - } + // Tar entries always use forward slashes; normalize on the tar side. + rel := stdpath.Clean(strings.TrimPrefix(header.Name, stdpath.Base(path)+"/")) + if rel == "." || rel == "" { + continue + } + if !filepath.IsLocal(rel) { + return errors.Errorf("tar entry %q escapes destination %q - refusing to copy", header.Name, path) + } + cleanedPath := filepath.FromSlash(rel)You'll also want
stdpath "path"in the import block alongside"path/filepath".Could you confirm the intent and run a quick check that no other call site relies on the current (broken) shape of the guard?
#!/bin/bash # Look for other places that mirror the same SplitList-based traversal check. rg -nP -C3 'filepath\.SplitList\([^)]+\)' # Look for existing usage of IsLocal so we can stay consistent if it's already in use. rg -nP -C2 '\bfilepath\.IsLocal\b' # Confirm CopyFromContainer callers to understand the blast radius if a malicious tar slips through. rg -nP -C3 '\bCopyFromContainer\s*\(' --type=goAs per coding guidelines: "Use crossplane-runtime/pkg/errors for wrapping ... CRITICAL: Ensure all error messages are meaningful to end users, not just developers ... 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/docker/docker.go` around lines 509 - 512, The current zip-slip guard using filepath.SplitList on cleanedPath is ineffective (it splits PATH lists, not path components); update the check in the extraction logic that computes cleanedPath (derived from header.Name and filepath.Base(path)) to validate escapes correctly: normalize the tar entry using the path package (e.g., stdpath.Clean on header.Name), then either use filepath.IsLocal on the converted-local path (Go 1.20+) or resolve the destination absolute path and the would-be extraction absolute path and ensure the child path has the root as a prefix (e.g., absChild starts with absRoot+os.PathSeparator); adjust imports to include "path" (as stdpath) and ensure the guard references cleaned/normalized tar-side name and rejects absolute or `..` escapes before writing, and run the suggested ripgrep checks (CopyFromContainer, filepath.IsLocal, filepath.SplitList) to ensure no other call sites rely on the broken check.
🧹 Nitpick comments (5)
internal/project/functions/go.go (1)
132-138: 💤 Low valueConsider documenting the pinned base image tag for easier future bumps.
Pinning by digest is great for reproducibility — thanks for that! One small nit: future maintainers (or a Renovate/Dependabot config) will have a hard time knowing which distroless tag this digest currently corresponds to when the time comes to bump it. Would you be open to a short comment noting the resolved tag (and maybe a link to the upstream image), or wiring this into your dependency-update tooling?
📝 Example annotation
func newGoBuilder(imageConfigs []pkgv1beta1.ImageConfig) *goBuilder { return &goBuilder{ + // gcr.io/distroless/static-debian12:nonroot as of <date>. Bump the digest + // when a new distroless release is published. baseImage: "gcr.io/distroless/static-debian12@sha256:a9329520abc449e3b14d5bc3a6ffae065bdde0f02667fa10880c49b35c109fd1",Totally optional — feel free to ignore if you already have tooling that tracks this.
🤖 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 132 - 138, The pinned distroless base image in newGoBuilder (goBuilder.baseImage) should be annotated so future maintainers can see the resolved tag and source; add a short comment above the baseImage assignment that states the human-readable tag (e.g., "distroless/static-debian12:TAG") and a link to the upstream image or release page, and optionally note the digest used, so dependency-update tooling or reviewers can easily map the digest to the tag when bumping.internal/project/install_test.go (2)
133-138: ⚡ Quick winPlease standardize table case shape to
args/wantand include areasonfield.These tables are clear already—thank you. A small structure tweak to the canonical
args/wantform (plusreason) would make them fully consistent with other tests and review expectations.As per coding guidelines "
**/*_test.go: Enforce table-driven test structure: ... args/want pattern ... Ensure proper test case naming and reason fields."Also applies to: 184-187, 210-213, 237-240
🤖 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/install_test.go` around lines 133 - 138, The table-driven test cases in install_test.go use top-level keys (source/constraint/wantName/wantFound); change the map values to the canonical args/want shape and add a reason string: replace the anonymous struct with struct { args struct { source string; constraint string }; want struct { name string; found bool }; reason string } and update all test case entries (including the other tables noted) to populate args.source/args.constraint, want.name/want.found and reason; then update test logic that references tcs[...] fields to use tc.args.source, tc.args.constraint, tc.want.name and tc.want.found (and include tc.reason in the test name or t.Run call).
49-115: ⚡ Quick winCould we align
TestApplyResourceswith the repo’s table-driven test pattern?Thanks for adding strong coverage here. Would you be open to converting this block into a single table (
args/want+ per-casereason) so it matches the project’s standard and is easier to extend with more apply edge cases?As per coding guidelines "
**/*_test.go: Enforce table-driven test structure: ... args/want pattern ... Ensure proper test case naming and reason fields."🤖 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/install_test.go` around lines 49 - 115, TestApplyResources uses three separate t.Run blocks instead of the repo's required table-driven pattern; refactor it into one table-driven test that iterates over cases with fields like name, args (including resources and client init), want (error expectation) and reason. Consolidate the current subtests ("AppliesConfigMaps", "IdempotentOnExisting", "EmptyRawErrors") into entries in the table, reuse helpers such as configMapJSON and newFakeClient, and call ApplyResources(t.Context(), cl, resources) inside the loop while using t.Run(tc.name, func(t *testing.T){...}) for each case; ensure each case asserts expected success/failure (checking created ConfigMaps or error presence) and includes a human-readable reason field.cmd/crossplane/project/push.go (1)
86-88: ⚡ Quick winConsider setting a minimum TLS version for defense-in-depth.
While
InsecureSkipVerifydisables certificate validation when enabled, settingMinVersion: tls.VersionTLS13in the TLS config would ensure that when certificate verification is enabled (the default case), only modern TLS versions are accepted. This follows security best practices even though it doesn't affect the insecure mode.♻️ Suggested enhancement
if c.InsecureSkipTLSVerify { t.TLSClientConfig = &tls.Config{ InsecureSkipVerify: true, //nolint:gosec // we need to support insecure connections if requested + MinVersion: tls.VersionTLS13, } + } else { + t.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS13, + } }🤖 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 86 - 88, The TLS config created in t.TLSClientConfig currently only sets InsecureSkipVerify, so add a minimum TLS version for defense-in-depth by setting MinVersion: tls.VersionTLS13 on the tls.Config; update the tls.Config construction used for t.TLSClientConfig (the tls.Config literal where InsecureSkipVerify is set) to include MinVersion: tls.VersionTLS13 so that when certificate verification is enabled modern TLS is required.internal/docker/docker.go (1)
516-533: ⚡ Quick winWrap filesystem errors with user-facing context.
These error returns currently propagate raw
aferoerrors (MkdirAll,Create,io.Copy) without any indication of what the user was doing or which file was involved — a bit of a debugging cliff for end users. Could we wrap them, similar to the rest of the file? Something like:♻️ Suggested wrapping
case tar.TypeDir: - if err := fs.MkdirAll(cleanedPath, 0o755); err != nil { - return err - } + if err := fs.MkdirAll(cleanedPath, 0o755); err != nil { + return errors.Wrapf(err, "failed to create directory %q while extracting from container", cleanedPath) + } case tar.TypeReg: - outFile, err := fs.Create(cleanedPath) - if err != nil { - return err - } + outFile, err := fs.Create(cleanedPath) + if err != nil { + return errors.Wrapf(err, "failed to create file %q while extracting from container", cleanedPath) + } - if _, err := io.Copy(outFile, tarReader); err != nil { //nolint:gosec // File size is checked above. + if _, err := io.Copy(outFile, tarReader); err != nil { //nolint:gosec // File size is checked above. if cerr := outFile.Close(); cerr != nil { err = errors.Wrap(cerr, "error while closing file") } - return err + return errors.Wrapf(err, "failed to write file %q from container tarball", cleanedPath) }As per coding guidelines: "Use crossplane-runtime/pkg/errors for wrapping ... 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."
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:
84d4c9ba-a012-4bf3-a733-64afc62c03cf⛔ 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)
- internal/dependency/cache_dir.go
🚧 Files skipped from review as they are similar to previous changes (64)
- cmd/crossplane/function/templates/python/README.md
- internal/schemas/generator/json_test.go
- internal/schemas/generator/kcl_test.go
- cmd/crossplane/xrd/xrd.go
- cmd/crossplane/function/function.go
- cmd/crossplane/dependency/auth.go
- internal/project/push_test.go
- cmd/crossplane/composition/composition.go
- cmd/crossplane/project/stop.go
- internal/schemas/generator/go_test.go
- cmd/crossplane/render/op/cmd_test.go
- cmd/crossplane/dependency/dependency.go
- internal/crd/generator.go
- internal/schemas/generator/interface.go
- cmd/crossplane/composition/generate_test.go
- internal/kcl/import_test.go
- cmd/crossplane/main.go
- cmd/crossplane/project/project.go
- internal/project/projectfile/projectfile_test.go
- internal/crd/generator_test.go
- cmd/crossplane/function/pipeline.go
- cmd/crossplane/xrd/generate_test.go
- cmd/crossplane/project/init.go
- internal/git/git_test.go
- cmd/crossplane/function/generate.go
- cmd/crossplane/dependency/cache.go
- internal/project/image.go
- cmd/crossplane/render/xr/cmd_test.go
- cmd/crossplane/project/run.go
- internal/async/event.go
- internal/project/functions/python.go
- internal/project/push.go
- cmd/crossplane/dependency/add.go
- internal/project/functions/build_test.go
- cmd/crossplane/render/op/cmd.go
- internal/project/projectfile/projectfile.go
- internal/project/helm/restclientgetter.go
- internal/schemas/generator/json.go
- internal/project/certs/tls.go
- internal/crd/convert_test.go
- internal/kcl/import.go
- cmd/crossplane/function/generate_test.go
- internal/project/build.go
- internal/project/helm/helm.go
- internal/project/functions/build.go
- internal/project/render.go
- internal/project/install.go
- internal/dependency/manager_test.go
- internal/schemas/generator/python.go
- internal/project/functions/go_templating.go
- internal/crd/convert.go
- internal/project/build_test.go
- internal/project/functions/kcl.go
- internal/dependency/manager.go
- internal/project/controlplane/controlplane.go
- internal/project/certs/cert_generator.go
- cmd/crossplane/project/build.go
- cmd/crossplane/composition/generate.go
- internal/filesystem/filesystem.go
- internal/git/git.go
- cmd/crossplane/xrd/generate.go
- internal/filesystem/filesystem_test.go
- internal/schemas/generator/kcl.go
- internal/schemas/generator/go.go
| func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinter) error { //nolint:gocognit // Orchestration is inherently complex. | ||
| ctx, cancel := context.WithTimeout(context.Background(), c.Timeout) | ||
| defer cancel() |
There was a problem hiding this comment.
Consider starting the timeout after project bootstrap.
Thanks for consolidating the context handling. The new placement means the default 1m budget now starts before loadFunctions(), so project-mode renders spend that same timeout on dependency resolution, schema generation, and embedded-function builds before the render engine even starts. On a cold cache or first build, that makes deadline exceeded a pretty likely first-run failure. Could we either start the timeout after function loading or split build and render budgets? As per coding guidelines **/cmd/**: "Ask about backward compatibility and user experience."
Also applies to: 242-243, 506-530
🤖 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 196 - 198, The timeout is being
started at the top of Cmd.Run which lets the default c.Timeout expire during
pre-render work (loadFunctions, dependency resolution, schema/gen, builds);
change Run so the initial phase runs with a non-deadlined context
(context.Background or ctx from kong) and only create context.WithTimeout for
the actual render phase (after loadFunctions() completes) or else introduce two
distinct timeouts (e.g., buildTimeout for loadFunctions/builds and renderTimeout
for the render engine) and wire them where used; update references around
Cmd.Run (the Run method), and the call sites that use the ctx for loadFunctions,
dependency resolution, and render so that build steps use the pre-timeout
context and only rendering uses the timed ctx.
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>
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.