Skip to content

feat: add xpkg get-crds subcommand#26

Merged
adamwg merged 9 commits into
crossplane:mainfrom
fernandezcuesta:feat/add-crd-download
Jun 15, 2026
Merged

feat: add xpkg get-crds subcommand#26
adamwg merged 9 commits into
crossplane:mainfrom
fernandezcuesta:feat/add-crd-download

Conversation

@fernandezcuesta

@fernandezcuesta fernandezcuesta commented May 22, 2026

Copy link
Copy Markdown
Contributor

Description of your changes

Add a crossplane xpkg get-crds subcommand to retrieve the CRD (and optionally, JSON schemas) of all dependencies (providers, functions).

Fixes #15

I have:

Happy to do a PR for the docs, but I see the docs are not update with the last CLI changes?

  • [ ] Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
@fernandezcuesta fernandezcuesta marked this pull request as ready for review May 22, 2026 07:29
@fernandezcuesta fernandezcuesta requested review from a team, jcogilvie and tampakrap as code owners May 22, 2026 07:29
@fernandezcuesta fernandezcuesta requested review from bobh66 and removed request for a team May 22, 2026 07:29
@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ef442909-1512-4a29-b257-15beb375b39a

📥 Commits

Reviewing files that changed from the base of the PR and between 4b38891 and 1e5b0c8.

📒 Files selected for processing (1)
  • internal/schemas/generator/json.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/schemas/generator/json.go

📝 Walkthrough

Walkthrough

This PR adds a xpkg get-crds subcommand that downloads and writes Crossplane package CRDs to an output directory, with options to convert to JSON Schema and control layout and caching. It exports a ToJSONSchema utility for conditional GVK metadata, exposes Manager.CRDs() accessor, and includes tests and documentation.

Changes

Get-CRDs Subcommand with Schema Export

Layer / File(s) Summary
JSON Schema export utility
internal/schemas/generator/json.go
Exports ToJSONSchema function that converts input schemas and conditionally sets $id and x-kubernetes-group-version-kind metadata when GroupVersionKind is provided; CRD and OpenAPI paths call with empty GVK to preserve existing behavior. Adds fmt import for ID formatting.
Manager CRDs accessor
cmd/crossplane/validate/manager.go
Exposes CRDs() accessor method to return collected CRDs slice.
Get-CRDs command implementation
cmd/crossplane/xpkg/get_crds.go
Implements getCRDsCmd with cache control, output layout, and schema conversion flags. Run method loads extensions, configures caching and temp directories, manages CRD resolution, and routes to writeCRDs (YAML output) or writeJSONSchemas (JSON Schema conversion). Includes helpers for storage version detection and output path computation (flat vs. group/version/kind layout).
xpkg command integration
cmd/crossplane/xpkg/xpkg.go
Wires get-crds subcommand into exported xpkg Cmd struct; reformats blank embed import.
Get-CRDs tests
cmd/crossplane/xpkg/get_crds_test.go
Unit tests with in-memory filesystem. TestWriteCRDs validates YAML output for structured/flat modes and empty lists. TestWriteJSONSchemas validates JSON Schema output, skips missing OpenAPI schemas, and validates generated files are valid JSON Schemas.
Get-CRDs documentation
cmd/crossplane/xpkg/help/get-crds.md
User documentation: describes CRD download, optional JSON Schema extraction, output layout options, accepted input sources, and usage examples with cache control.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • tampakrap
  • jcogilvie
  • bobh66
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a new xpkg get-crds subcommand, and it stays well under the 72-character limit at 34 characters.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose of the new subcommand and referencing the issue it fixes, along with completion of contribution checklist items.
Linked Issues check ✅ Passed The PR fully addresses issue #15 requirements: implements a command to download CRDs to a local folder, supports JSON schema export via --json-schema flag, replicates validate functionality, and is placed under xpkg as suggested.
Out of Scope Changes check ✅ Passed All changes are in scope: new get-crds subcommand implementation, Manager.CRDs() getter, xpkg command integration, JSON schema conversion enhancements, and comprehensive tests—all directly supporting the CRD download feature objective.
Breaking Changes ✅ Passed PR contains only additive changes: new methods, functions, and struct fields for CLI subcommand registration. No public APIs were removed, renamed, or made required without breaking-change label.
Feature Gate Requirement ✅ Passed PR adds new get-crds subcommand and ToJSONSchema API, neither marked as experimental, no apis/** modifications, and doesn't modify existing behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
cmd/crossplane/validate/manager.go (1)

80-83: ⚡ Quick win

Avoid exposing mutable manager state from CRDs().

Could we return a copied slice here so callers can’t accidentally mutate Manager’s internal state?

Proposed change
 func (m *Manager) CRDs() []*extv1.CustomResourceDefinition {
-	return m.crds
+	out := make([]*extv1.CustomResourceDefinition, len(m.crds))
+	copy(out, m.crds)
+	return out
 }
🤖 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/validate/manager.go` around lines 80 - 83, The CRDs() accessor
currently returns the internal slice m.crds allowing external mutation; change
CRDs() to return a shallow copy of the slice (e.g., allocate a new slice with
the same length and copy m.crds into it) so callers cannot mutate Manager's
internal slice while still returning the same []*extv1.CustomResourceDefinition
elements.
cmd/crossplane/xpkg/crd_test.go (1)

147-147: ⚡ Quick win

Use cmpopts.EquateErrors() for error comparisons in these table tests.

Nice coverage overall. Could we align these three assertions with the repo’s test pattern for error equality?

Proposed change
 import (
 	"bytes"
 	"encoding/json"
 	"testing"
 
 	"github.com/alecthomas/kong"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/spf13/afero"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	"github.com/crossplane/crossplane-runtime/v2/pkg/test"
 )
...
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
...
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
...
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {

As per coding guidelines **/*_test.go: "use cmp.Diff with cmpopts.EquateErrors() for error testing."

Also applies to: 245-245, 330-330

🤖 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/xpkg/crd_test.go` at line 147, Replace the use of
cmp.EquateErrors() in the cmp.Diff error assertions with cmpopts.EquateErrors();
specifically update the cmp.Diff(...) calls that compare tc.want.err and err to
use cmpopts.EquateErrors() and ensure the cmpopts package is imported
(github.com/google/go-cmp/cmp/cmpopts) so the table-test error comparisons
follow the repo test pattern.
🤖 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/xpkg/crd.go`:
- Around line 114-117: The expansion of c.CacheDir when it starts with "~/"
currently ignores the error from os.UserHomeDir(), which can produce an invalid
path; update the code that handles c.CacheDir (the branch checking
strings.HasPrefix(c.CacheDir, "~/")) to check the returned error from
os.UserHomeDir(), and on error return/fail fast with a clear error message that
includes the underlying error and guidance to provide an absolute --cache-dir.
Ensure the change surfaces the error to the caller (rather than silently using
an empty homeDir) so callers of the function receive the failure.

---

Nitpick comments:
In `@cmd/crossplane/validate/manager.go`:
- Around line 80-83: The CRDs() accessor currently returns the internal slice
m.crds allowing external mutation; change CRDs() to return a shallow copy of the
slice (e.g., allocate a new slice with the same length and copy m.crds into it)
so callers cannot mutate Manager's internal slice while still returning the same
[]*extv1.CustomResourceDefinition elements.

In `@cmd/crossplane/xpkg/crd_test.go`:
- Line 147: Replace the use of cmp.EquateErrors() in the cmp.Diff error
assertions with cmpopts.EquateErrors(); specifically update the cmp.Diff(...)
calls that compare tc.want.err and err to use cmpopts.EquateErrors() and ensure
the cmpopts package is imported (github.com/google/go-cmp/cmp/cmpopts) so the
table-test error comparisons follow the repo test pattern.
🪄 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: b85eaa87-430f-46a4-ab19-94788ce42e99

📥 Commits

Reviewing files that changed from the base of the PR and between 5a1ea69 and 73e1dbd.

📒 Files selected for processing (4)
  • cmd/crossplane/validate/manager.go
  • cmd/crossplane/xpkg/crd.go
  • cmd/crossplane/xpkg/crd_test.go
  • cmd/crossplane/xpkg/xpkg.go

Comment on lines +114 to +117
} else if strings.HasPrefix(c.CacheDir, "~/") {
homeDir, _ := os.UserHomeDir()
c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="cmd/crossplane/xpkg/crd.go"

# Show the surrounding lines for the ~/ cache-dir expansion
nl -ba "$FILE" | sed -n '90,150p'

# Show imports to confirm which errors package is used in this file
echo "---- imports ----"
sed -n '1,80p' "$FILE"

# Search for UserHomeDir usage in this file/repo for consistency
rg -n "UserHomeDir\(" -S "$FILE" || true
rg -n "cannot resolve home directory|cache-dir to an absolute path" -S "$FILE" || true

# Check if errors.Wrap/Wrapf from crossplane-runtime is used around return-early patterns
rg -n "errors\.Wrapf?\(" "$FILE" | head -n 50

Repository: crossplane/cli

Length of output: 101


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="cmd/crossplane/xpkg/crd.go"

# Print lines 90-150 with line numbers via awk (since nl isn't available)
echo "---- snippet (lines 90-150) ----"
awk -v start=90 -v end=150 'NR>=start && NR<=end {printf "%4d:%s\n", NR, $0}' "$FILE"

echo "---- imports (lines 1-80) ----"
sed -n '1,80p' "$FILE"

echo "---- UserHomeDir usage in this file ----"
rg -n "UserHomeDir\(" "$FILE" || true

echo "---- errors.Wrap/Wrapf usage in this file ----"
rg -n "errors\.Wrapf?\(" "$FILE" || true

echo "---- any mention of cache-dir / CacheDir in this file ----"
rg -n "cache-dir|CacheDir" "$FILE" || true

Repository: crossplane/cli

Length of output: 7670


Handle os.UserHomeDir() failure when expanding ~/ in --cache-dir — currently the error is ignored, so a bad/empty home dir can produce a broken cache path without user feedback. Should crossplane xpkg crd fail fast here with guidance to pass an absolute --cache-dir?

Proposed change
 	} else if strings.HasPrefix(c.CacheDir, "~/") {
-		homeDir, _ := os.UserHomeDir()
+		homeDir, err := os.UserHomeDir()
+		if err != nil {
+			return errors.Wrap(err, "cannot resolve home directory; set --cache-dir to an absolute path")
+		}
 		c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
 	}
📝 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.

Suggested change
} else if strings.HasPrefix(c.CacheDir, "~/") {
homeDir, _ := os.UserHomeDir()
c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
}
} else if strings.HasPrefix(c.CacheDir, "~/") {
homeDir, err := os.UserHomeDir()
if err != nil {
return errors.Wrap(err, "cannot resolve home directory; set --cache-dir to an absolute path")
}
c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
}
🤖 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/xpkg/crd.go` around lines 114 - 117, The expansion of
c.CacheDir when it starts with "~/" currently ignores the error from
os.UserHomeDir(), which can produce an invalid path; update the code that
handles c.CacheDir (the branch checking strings.HasPrefix(c.CacheDir, "~/")) to
check the returned error from os.UserHomeDir(), and on error return/fail fast
with a clear error message that includes the underlying error and guidance to
provide an absolute --cache-dir. Ensure the change surfaces the error to the
caller (rather than silently using an empty homeDir) so callers of the function
receive the failure.

@adamwg adamwg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thanks for the contribution! This is definitely a useful feature.

That said, there are a few high-level things I'd like to consider before reviewing this in detail:

  1. The functionality here feels very close to crossplane xpkg extract in that it fetches xpkgs and extracts their contents (though it does very different things with that extracted contents). I wonder whether writing CRDs and JSONSchemas should become a feature of extract rather than a separate command.
  2. We've also implemented OpenAPI -> JSONSchema conversion in internal/schemas/generator. It would be nice to re-use that code, which also handles a bunch of nuances to make the resulting JSONSchemas work super well with the YAML language server.
  3. It's on my TODO list to change crossplane validate to use the shared infrastructure from internal/xpkg, which would also make it use the shared xpkg cache used by the crossplane dependency commands. It may actually make sense to use the existing dependency.Manager in validate, removing its custom Manager entirely. I don't think there's much harm in this PR introducing another usage of the validate.Manager (we can swap it out at the same time when we update validate), but I wonder if using the dependency.Manager would simplify things a bit here and also reduce the amount of consolidation needed in the future.

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
@fernandezcuesta

Copy link
Copy Markdown
Contributor Author

Hi @adamwg, thanks for taking a look.

Regarding your points:

  1. Fully agree, moved it under crossplane xpkg crd. I'm still getting familiar with the command structure. Since the input is a crossplane.yaml file, I think it doesn't fit xpkg extract.
  2. Refactor to reuse as much as possible. I made some slight changes to oapiSchemaToJSONSchema (apart from exposing it) to add $id and gvk info. Not fully needed but can be a useful addition IMO.
  3. Agree this would be a better long term solution. Since you're already planning to move validate to the shared internal/xpkg infrastructure, I think the pragmatic path is to keep validate.Manager here for now and swap both at the same time. Happy to take on either approach though.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmd/crossplane/xpkg/crd_test.go (1)

181-184: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don’t ignore filesystem errors in test assertions.

Thanks for adding these coverage paths. Right now afero.Exists / afero.ReadFile errors are discarded, which can mask real test failures; can we fail the test when those calls error?

Proposed patch
-			for _, f := range tc.want.files {
-				exists, _ := afero.Exists(fs, f)
+			for _, f := range tc.want.files {
+				exists, err := afero.Exists(fs, f)
+				if err != nil {
+					t.Fatalf("%s\nwriteCRDs(...): failed checking file %s: %v", tc.reason, f, err)
+				}
 				if !exists {
 					t.Errorf("%s\nwriteCRDs(...): expected file %s to exist", tc.reason, f)
 				}
 			}
@@
-			for _, f := range tc.want.files {
-				exists, _ := afero.Exists(fs, f)
+			for _, f := range tc.want.files {
+				exists, err := afero.Exists(fs, f)
+				if err != nil {
+					t.Fatalf("%s\nwriteJSONSchemas(...): failed checking file %s: %v", tc.reason, f, err)
+				}
 				if !exists {
 					t.Errorf("%s\nwriteJSONSchemas(...): expected file %s to exist", tc.reason, f)
 				}
 
-				data, _ := afero.ReadFile(fs, f)
+				data, err := afero.ReadFile(fs, f)
+				if err != nil {
+					t.Fatalf("%s\nwriteJSONSchemas(...): failed reading file %s: %v", tc.reason, f, err)
+				}
 				var schema jsonschema.Schema
 				if err := json.Unmarshal(data, &schema); err != nil {
 					t.Errorf("%s\nwriteJSONSchemas(...): file %s is not valid JSON Schema: %v", tc.reason, f, err)
 				}
 			}

Also applies to: 279-285

🤖 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/xpkg/crd_test.go` around lines 181 - 184, The test currently
ignores errors returned by afero.Exists and afero.ReadFile which can hide real
failures; update the assertions in the test (the block using afero.Exists(fs, f)
and the later block using afero.ReadFile(fs, f)) to check the returned error and
fail the test when err != nil (use t.Fatalf or t.Fatalf-like assertion) instead
of discarding the error, and only then assert on the existence/contents
(referencing the variables fs, f, exists and the ReadFile call) so any
filesystem error surfaces as a test failure.
🧹 Nitpick comments (2)
cmd/crossplane/xpkg/crd_test.go (1)

176-176: ⚡ Quick win

Align error assertions with the repo’s test pattern (cmpopts.EquateErrors).

Nice table setup overall. Could you switch these comparisons to cmp.Diff(..., cmpopts.EquateErrors()) to match the required _test.go convention consistently?

Proposed patch
 import (
 	"bytes"
 	"encoding/json"
 	"testing"
 
 	"github.com/alecthomas/kong"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/invopop/jsonschema"
 	"github.com/spf13/afero"
@@
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
 				t.Errorf("%s\nwriteCRDs(...): -want error, +got error:\n%s", tc.reason, diff)
 			}
@@
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
 				t.Errorf("%s\nwriteJSONSchemas(...): -want error, +got error:\n%s", tc.reason, diff)
 			}

As per coding guidelines, "**/*_test.go: ... use cmp.Diff with cmpopts.EquateErrors() for error testing."

Also applies to: 274-274

🤖 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/xpkg/crd_test.go` at line 176, Replace the use of
test.EquateErrors() in the cmp.Diff error assertions with cmpopts.EquateErrors()
(i.e., change cmp.Diff(tc.want.err, err, test.EquateErrors()) to
cmp.Diff(tc.want.err, err, cmpopts.EquateErrors())), and add the required import
for the cmpopts package (github.com/google/go-cmp/cmp/cmpopts) so the test
follows the repo convention; update both occurrences referenced (around the
cmp.Diff calls).
internal/schemas/generator/json.go (1)

102-112: ⚡ Quick win

Re: Preserve existing jsonschema.Schema extras when adding GVK metadata

internal/schemas/generator/json.go overwrites conv.Extras when group/version/kind are set, but with invopop/jsonschema v0.14.0 this is unlikely to drop anything: Schema.Extras is json:"-" and Schema.UnmarshalJSON just unmarshals into SchemaAlt, so unknown x-* fields from the input JSON won’t populate conv.Extras in the first place. Since this helper also doesn’t set conv.Extras anywhere else before that block, it should be safe today; merging could still be a future-proof tweak if other code starts populating conv.Extras earlier.

🤖 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/json.go` around lines 102 - 112, The code
overwrites conv.Extras when setting GVK metadata which could clobber any
existing extras; modify the block that sets conv.Extras (where conv.ID is set
and GVK map is created) to preserve existing conv.Extras by copying or merging
into the existing map instead of assigning a new map, and ensure the
"x-kubernetes-group-version-kind" key is added or replaced within conv.Extras;
refer to conv.Extras and the GVK-assignment block where conv.ID is set and merge
the new []map[string]string value into the existing conv.Extras map rather than
replacing it.
🤖 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.

Outside diff comments:
In `@cmd/crossplane/xpkg/crd_test.go`:
- Around line 181-184: The test currently ignores errors returned by
afero.Exists and afero.ReadFile which can hide real failures; update the
assertions in the test (the block using afero.Exists(fs, f) and the later block
using afero.ReadFile(fs, f)) to check the returned error and fail the test when
err != nil (use t.Fatalf or t.Fatalf-like assertion) instead of discarding the
error, and only then assert on the existence/contents (referencing the variables
fs, f, exists and the ReadFile call) so any filesystem error surfaces as a test
failure.

---

Nitpick comments:
In `@cmd/crossplane/xpkg/crd_test.go`:
- Line 176: Replace the use of test.EquateErrors() in the cmp.Diff error
assertions with cmpopts.EquateErrors() (i.e., change cmp.Diff(tc.want.err, err,
test.EquateErrors()) to cmp.Diff(tc.want.err, err, cmpopts.EquateErrors())), and
add the required import for the cmpopts package
(github.com/google/go-cmp/cmp/cmpopts) so the test follows the repo convention;
update both occurrences referenced (around the cmp.Diff calls).

In `@internal/schemas/generator/json.go`:
- Around line 102-112: The code overwrites conv.Extras when setting GVK metadata
which could clobber any existing extras; modify the block that sets conv.Extras
(where conv.ID is set and GVK map is created) to preserve existing conv.Extras
by copying or merging into the existing map instead of assigning a new map, and
ensure the "x-kubernetes-group-version-kind" key is added or replaced within
conv.Extras; refer to conv.Extras and the GVK-assignment block where conv.ID is
set and merge the new []map[string]string value into the existing conv.Extras
map rather than replacing it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3c7779b1-ea69-4672-b009-0eb3e6f33dc4

📥 Commits

Reviewing files that changed from the base of the PR and between 73e1dbd and 97c1b1a.

📒 Files selected for processing (3)
  • cmd/crossplane/xpkg/crd.go
  • cmd/crossplane/xpkg/crd_test.go
  • internal/schemas/generator/json.go

@adamwg adamwg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but this looks good to me overall - thanks for the updates! Happy to leave it using the cmd/validate manager/cache for now and refactor to use the shared xpkg implementation in the future.

Comment thread internal/schemas/generator/json.go Outdated
Comment thread cmd/crossplane/xpkg/xpkg.go Outdated
Comment thread cmd/crossplane/xpkg/crd.go Outdated
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
@fernandezcuesta fernandezcuesta changed the title feat: add resource crd subcommand feat: add xpkg get-crds subcommand Jun 2, 2026

@adamwg adamwg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending CI.

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
cmd/crossplane/xpkg/get_crds.go (1)

95-98: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Handle os.UserHomeDir() failure when expanding ~/ in --cache-dir.

This issue was previously flagged: when os.UserHomeDir() fails, the error is ignored and an empty/invalid home directory can silently produce a broken cache path. The command should fail fast here with guidance for the user to provide an absolute --cache-dir path.

🔒 Proposed fix
 } else if strings.HasPrefix(c.CacheDir, "~/") {
-	homeDir, _ := os.UserHomeDir()
+	homeDir, err := os.UserHomeDir()
+	if err != nil {
+		return errors.Wrap(err, "cannot resolve home directory; set --cache-dir to an absolute path")
+	}
 	c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
 }
🤖 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/xpkg/get_crds.go` around lines 95 - 98, The code that expands
~/ in the cache-dir (the block checking strings.HasPrefix(c.CacheDir, "~/") in
get_crds.go) ignores the error from os.UserHomeDir(); update that block to check
the error returned by os.UserHomeDir() and fail fast (return an error or
log/fatal) if it fails, instead of using an empty homeDir; include a clear
message instructing the user to provide an absolute --cache-dir path when home
lookup fails and ensure c.CacheDir is only rewritten to filepath.Join(homeDir,
c.CacheDir[2:]) on success.
🤖 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.

Duplicate comments:
In `@cmd/crossplane/xpkg/get_crds.go`:
- Around line 95-98: The code that expands ~/ in the cache-dir (the block
checking strings.HasPrefix(c.CacheDir, "~/") in get_crds.go) ignores the error
from os.UserHomeDir(); update that block to check the error returned by
os.UserHomeDir() and fail fast (return an error or log/fatal) if it fails,
instead of using an empty homeDir; include a clear message instructing the user
to provide an absolute --cache-dir path when home lookup fails and ensure
c.CacheDir is only rewritten to filepath.Join(homeDir, c.CacheDir[2:]) on
success.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 81c15beb-6bda-4047-9c21-e145a5055456

📥 Commits

Reviewing files that changed from the base of the PR and between 97c1b1a and d7bd755.

📒 Files selected for processing (5)
  • cmd/crossplane/xpkg/get_crds.go
  • cmd/crossplane/xpkg/get_crds_test.go
  • cmd/crossplane/xpkg/help/get-crds.md
  • cmd/crossplane/xpkg/xpkg.go
  • internal/schemas/generator/json.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/crossplane/xpkg/xpkg.go

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
@fernandezcuesta

Copy link
Copy Markdown
Contributor Author

lint errors should be fine now 🤞

@fernandezcuesta

fernandezcuesta commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@adamwg could you please approve to run the workfows? I expect this time to be compliant 🤞

Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
@adamwg adamwg merged commit 3df82fc into crossplane:main Jun 15, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Command to download dependencies CRDs

2 participants