feat: add resource crd subcommand#26
Open
fernandezcuesta wants to merge 2 commits into
Open
Conversation
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
fernandezcuesta
requested review from
a team,
jcogilvie and
tampakrap
as code owners
📝 WalkthroughWalkthroughThis PR adds an ChangesCRD Export Subcommand
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/crossplane/validate/manager.go (1)
80-83: ⚡ Quick winAvoid 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 winUse
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
📒 Files selected for processing (4)
cmd/crossplane/validate/manager.gocmd/crossplane/xpkg/crd.gocmd/crossplane/xpkg/crd_test.gocmd/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:]) | ||
| } |
There was a problem hiding this comment.
🧩 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 50Repository: 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" || trueRepository: 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
reviewed
May 26, 2026
Member
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
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:
- The functionality here feels very close to
crossplane xpkg extractin 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 ofextractrather than a separate command. - 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. - It's on my TODO list to change
crossplane validateto use the shared infrastructure frominternal/xpkg, which would also make it use the shared xpkg cache used by thecrossplane dependencycommands. It may actually make sense to use the existingdependency.Managerin validate, removing its customManagerentirely. I don't think there's much harm in this PR introducing another usage of thevalidate.Manager(we can swap it out at the same time when we updatevalidate), but I wonder if using thedependency.Managerwould 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>
Author
|
Hi @adamwg, thanks for taking a look. Regarding your points:
|
There was a problem hiding this comment.
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 winDon’t ignore filesystem errors in test assertions.
Thanks for adding these coverage paths. Right now
afero.Exists/afero.ReadFileerrors 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 winAlign 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.goconvention 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 winRe: Preserve existing
jsonschema.Schemaextras when adding GVK metadata
internal/schemas/generator/json.gooverwritesconv.Extraswhengroup/version/kindare set, but withinvopop/jsonschemav0.14.0 this is unlikely to drop anything:Schema.Extrasisjson:"-"andSchema.UnmarshalJSONjust unmarshals intoSchemaAlt, so unknownx-*fields from the input JSON won’t populateconv.Extrasin the first place. Since this helper also doesn’t setconv.Extrasanywhere else before that block, it should be safe today; merging could still be a future-proof tweak if other code starts populatingconv.Extrasearlier.🤖 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
📒 Files selected for processing (3)
cmd/crossplane/xpkg/crd.gocmd/crossplane/xpkg/crd_test.gointernal/schemas/generator/json.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Add a
crossplane xpkg crdsubcommand to retrieve the CRD (and optionally, JSON schemas) of all dependencies (providers, functions).Fixes #15
I have:
./nix.sh flake checkto ensure this PR is ready for review.[ ] Linked a PR or a docs tracking issue to document this change.[ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.