feat: support for advertised capabilities and required schemas#256
feat: support for advertised capabilities and required schemas#256jbw976 wants to merge 4 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>
📝 WalkthroughWalkthroughThese changes introduce a capability advertisement and schema management system to the function execution protocol. New message types (SchemaSelector, Schema, Capability enum) are added to both v1 and v1beta1 protocol buffers, enabling functions to declare supported features and request OpenAPI schemas. Corresponding Go helper functions facilitate capability checking and schema retrieval/declaration, with comprehensive test coverage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
| for _, c := range req.GetMeta().GetCapabilities() { | ||
| if c == capability { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
I think slices.Contains(req.GetMeta().GetCapabilities(), capability) is the hip new way to do this.
| // Use AdvertisesCapabilities to check whether | ||
| // Crossplane advertises its capabilities at all. If it doesn't, HasCapability | ||
| // always returns false even for features the older Crossplane does support. | ||
| func HasCapability(req *v1.RunFunctionRequest, capability v1.Capability) bool { |
There was a problem hiding this comment.
| func HasCapability(req *v1.RunFunctionRequest, capability v1.Capability) bool { | |
| func HasCapability(req *v1.RunFunctionRequest, cap v1.Capability) bool { |
Nit: doesn't need a long var name given the descriptive type name.
| // 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.
2 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.