feat: support for advertised capabilities and required schemas#256
feat: support for advertised capabilities and required schemas#256jbw976 merged 6 commits intocrossplane:mainfrom
Conversation
…nd required schemas Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
📝 WalkthroughWalkthroughAdds capability advertisement and required-schema negotiation to function execution protos (v1 and v1beta1): new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Thanks — this looks complete; let me know if you want the diagram expanded (e.g., old vs new flow) or a focused review checklist for the proto and Go API changes. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
request/request.go
Outdated
| // a protobuf Struct, or nil if the schema was not found. Returns nil both when | ||
| // Crossplane has not yet resolved the requirement and when it was resolved but | ||
| // the schema wasn't found. To distinguish between these cases, check whether | ||
| // the name exists in req.GetRequiredSchemas(). |
There was a problem hiding this comment.
Worth adding GetRequiredSchemas (plural)? I'm on the fence. It'd be nice for documentation purposes and to stay consistent with GetRequiredResources but I imagine it wouldn't actually do much.
There was a problem hiding this comment.
To distinguish between these cases, check whether the name exists in req.GetRequiredSchemas().
It occurs to me that a better UX would be for this to return (*structpb.Struct, bool) with the bool indicating whether the resource was resolved. I'd like to stay consistent with GetRequiredResource though. I could be convinced a better UX is worth a breaking change here, given this SDK is pre-1.0.
There was a problem hiding this comment.
Sounds reasonable to have a (*structpb.Struct, bool) return here - I agree that's a better UX than the current "nil means two things" ambiguity. The (value, bool) pattern is more idiomatic Go, so it'll feel natural to consumers of this function.
For consistency across GetRequiredResource* and GetRequiredSchema*, let's do the following:
- Update
GetRequiredSchemato return(*structpb.Struct, bool)whereboolindicates whether Crossplane resolved the requirement - Add
GetRequiredSchemas(plural) for consistency withGetRequiredResources- likely a thin wrapper that doesn't do a ton - Add
GetRequiredResource(singular) with the same(value, bool)pattern to stay consistent withGetRequiredSchema. This singular version doesn't actually exist yet, so it doesn't require a signature change.
None of those are breaking changes because the only changing signature is in GetRequiredSchema which is new in this PR.
With the Python SDK, the get_required_schema API has already shipped in https://github.com/crossplane/function-sdk-python/releases/tag/v0.11.0, so changing anything there would indeed be a breaking change. However, Claude tells me that returning dict | None is already idiomatic Python for "not found." I'm good with each SDK following the conventions of its own language rather than mirror each other 1:1. Sound reasonable?
There was a problem hiding this comment.
i pushed a new commit that implemented the approach described above, let me know what folks think! 🙇♂️
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
request/request.go (1)
134-154: Nice API design — the three-value return matchesGetRequiredSchemawell.One small question: the error from
resource.AsObject(line 148) propagates unwrapped, which is consistent withGetRequiredResourcesabove (line 125). However, the coding guidelines ask that error messages include context about what the user was trying to do. Would it be worth wrapping with something likeerrors.Wrapf(err, "cannot get required resource %q", name)to help users diagnose which resource failed? Totally up to you — I can see the argument for consistency withGetRequiredResourcestoo.🔧 Optional: wrap the error for better diagnostics
for _, i := range rrs.GetItems() { r := &resource.Required{Resource: &unstructured.Unstructured{}} if err := resource.AsObject(i.GetResource(), r.Resource); err != nil { - return nil, true, err + return nil, true, errors.Wrapf(err, "cannot get required resource %q", name) } out = append(out, *r) }As per coding guidelines,
**/*.go: "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 the current code and only fix it if needed. In `@request/request.go` around lines 134 - 154, Wrap the error returned by resource.AsObject in GetRequiredResource with contextual information so users know which required resource failed to parse; replace the direct return of err (from resource.AsObject in the loop inside GetRequiredResource) with a wrapped error that includes the resource name (name) and a short message like "cannot get required resource %q" to improve diagnostics while preserving the original error.request/request_test.go (1)
469-553: Thorough test coverage forGetRequiredResource— nice work!The test cases cover the key scenarios well (no resources, wrong name, found). One thing I noticed: line 548 uses
cmp.Diff(tc.want.err, err)withoutcmpopts.EquateErrors(). This works because all current cases havenilerrors, but it would panic or give confusing diffs if you later add an error test case. I see existing tests in this file follow the same pattern, so this is pre-existing — just flagging for awareness. Could you confirm whether that's intentional or something to address across the board later?🔧 Optional: use cmpopts.EquateErrors() for future-proofing
+ "github.com/google/go-cmp/cmp/cmpopts"- if diff := cmp.Diff(tc.want.err, err); 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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@request/request_test.go` around lines 469 - 553, Update the error comparison in TestGetRequiredResource to use cmpopts.EquateErrors() so error diffs don't panic or misreport: change the last assertion to cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()) and add the cmpopts import ("github.com/google/go-cmp/cmp/cmpopts") at the top of the test file; refer to TestGetRequiredResource and the cmp.Diff(...) that compares tc.want.err and err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@request/request_test.go`:
- Around line 469-553: Update the error comparison in TestGetRequiredResource to
use cmpopts.EquateErrors() so error diffs don't panic or misreport: change the
last assertion to cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()) and add the
cmpopts import ("github.com/google/go-cmp/cmp/cmpopts") at the top of the test
file; refer to TestGetRequiredResource and the cmp.Diff(...) that compares
tc.want.err and err.
In `@request/request.go`:
- Around line 134-154: Wrap the error returned by resource.AsObject in
GetRequiredResource with contextual information so users know which required
resource failed to parse; replace the direct return of err (from
resource.AsObject in the loop inside GetRequiredResource) with a wrapped error
that includes the resource name (name) and a short message like "cannot get
required resource %q" to improve diagnostics while preserving the original
error.
…and GetRequiredSchema* Signed-off-by: Jared Watts <jbw976@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
request/request.go (1)
134-153: Consider extracting shared logic withGetRequiredResources.Thanks for this clean API addition! I notice that
GetRequiredResource(lines 136–153) andGetRequiredResources(lines 119–132) share nearly identical item-parsing logic. Would it make sense to extract the inner loop into a small helper (e.g.,parseRequiredItems) and have both functions call it? This would reduce the duplication and make future changes (e.g., populating additional fields onresource.Required) less error-prone.Totally fine to defer this if you'd prefer to keep the PR focused — just wanted to flag it for discussion.
♻️ Possible helper extraction
+func parseRequiredItems(rrs *v1.Resources) ([]resource.Required, error) { + out := make([]resource.Required, 0, len(rrs.GetItems())) + for _, i := range rrs.GetItems() { + r := &resource.Required{Resource: &unstructured.Unstructured{}} + if err := resource.AsObject(i.GetResource(), r.Resource); err != nil { + return nil, err + } + out = append(out, *r) + } + return out, nil +}Then
GetRequiredResourcesandGetRequiredResourcecan both delegate to it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@request/request.go` around lines 134 - 153, Extract the duplicated loop that converts proto items to []resource.Required into a helper (e.g., parseRequiredItems) and have both GetRequiredResources and GetRequiredResource call it; the helper should accept the proto Items slice (or the ResourceRequirementSet/rrs.GetItems()) and return ([]resource.Required, error), performing the current logic that allocates resource.Required with Resource: &unstructured.Unstructured{}, calls resource.AsObject on each i.GetResource(), and appends to the output slice, so callers only handle presence checks and the boolean resolution flag while reusing the shared parsing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@request/request.go`:
- Around line 134-153: Extract the duplicated loop that converts proto items to
[]resource.Required into a helper (e.g., parseRequiredItems) and have both
GetRequiredResources and GetRequiredResource call it; the helper should accept
the proto Items slice (or the ResourceRequirementSet/rrs.GetItems()) and return
([]resource.Required, error), performing the current logic that allocates
resource.Required with Resource: &unstructured.Unstructured{}, calls
resource.AsObject on each i.GetResource(), and appends to the output slice, so
callers only handle presence checks and the boolean resolution flag while
reusing the shared parsing code.
| } | ||
| } | ||
|
|
||
| func TestGetRequiredSchemas(t *testing.T) { |
There was a problem hiding this comment.
GetRequiredSchema has a dedicated SchemaResolvedButNotFound case, but the TestGetRequiredSchemas doesn't verify that an empty Schema{} maps to a nil value in the output. Worth adding since GetRequiredSchemas silently flattens that distinction:
"ResolvedButNotFound": {
reason: "A schema resolved but not found should appear as nil in the output map.",
req: &v1.RunFunctionRequest{
RequiredSchemas: map[string]*v1.Schema{"test": {}},
},
want: map[string]*structpb.Struct{"test": nil},
},
There was a problem hiding this comment.
cool, i'll add this test case!
request/request.go
Outdated
| // AdvertisesCapabilities returns true if Crossplane advertises its capabilities | ||
| // in the request metadata. Crossplane v2.2 and later advertise capabilities. If | ||
| // this returns false, the calling Crossplane predates capability advertisement | ||
| // and HasCapability will always return False, even for features the older |
There was a problem hiding this comment.
False should be lowercase false — it refers to the Go keyword
| for _, i := range rrs.GetItems() { | ||
| r := &resource.Required{Resource: &unstructured.Unstructured{}} | ||
| if err := resource.AsObject(i.GetResource(), r.Resource); err != nil { | ||
| return nil, true, err |
There was a problem hiding this comment.
in Line36 we wrapping the errors - is it fine here without context?
perhaps something like
return nil, true, errors.Wrap(err, "cannot convert required resource to object")
There was a problem hiding this comment.
i think coderabbit said the same thing - i was choosing to remain consistent with the error handling behavior of the existing GetRequiredResources() function. It does not wrap this same error either.
…ved but not found Signed-off-by: Jared Watts <jbw976@gmail.com>
3 participants
Description of your changes
Similar to the python SDK update in crossplane/function-sdk-python#193, this PR updates the Go SDK to support advertised capabilities and required schemas, which were added to Crossplane in crossplane/crossplane#7022.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
I have tested these changes locally in a function and verified that requesting schemas works as expected and capabilities are being advertised as expected.