-
Notifications
You must be signed in to change notification settings - Fork 25
Update to crossplane-runtime v2.0.0 #226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the initiative on this @twobiers!
Would you also be up for taking this update for a test drive in building a function, e.g. building on what's in https://docs.crossplane.io/v2.0/guides/write-a-composition-function-in-go/, to see if everything is working as expected there for namespaced resources? That would be really helpful 🙏
@twobiers did you get a chance to check if your PR works with the linked docs? |
I was able to successfully test this version (redirect to twobiers:v2) of the function sdk in some internal functions with namespaced and cluster scoped resource functions. |
@jbw976 can we bring this in and cut a new release ? |
No, I didn't have enough capacity yet for a full trial. If anyone wants to take this PR over, feel free to do so. |
Thanks for your efforts so far @twobiers! and thank you @bakito for your testing validation, really helpful! 🙇♂️ I rebased this PR from latest on main to fix merge conflicts, mostly around the removal of provider-aws dependency in #224. I also bumped go to v1.24.8 and golangci-lint to v2.4.0 consistently across Makefile and ci.yml. I've done some testing using some local |
@jbw976 can we merge the PR then ? |
Signed-off-by: twobiers <[email protected]>
Signed-off-by: twobiers <[email protected]>
Signed-off-by: twobiers <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
Just resolved more merge conflicts now - @haarchri will add some details for how he's tested this PR as well 💪 |
📝 WalkthroughWalkthroughUpgrades dependencies and imports to crossplane-runtime v2, adjusts CI/lint tooling versions and config, removes two PublishConnectionDetailsTo methods from composed and composite resources, adds a test for namespace-scoped required resources, and switches listener creation to net.ListenConfig in SDK and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SDK as SDK Server
participant Net as net.ListenConfig
User->>SDK: Start(options)
SDK->>Net: Listen(ctx, so.Network, so.Address)
alt listen ok
Net-->>SDK: net.Listener
SDK-->>User: Running with listener
else error
Net-->>SDK: error
SDK-->>User: return error
end
sequenceDiagram
autonumber
participant Fn as Function Logic
participant RR as RequiredResources
participant U as Unstructured
Fn->>RR: GetRequiredResources()
RR-->>Fn: items with {apiVersion, kind, metadata.name[, metadata.namespace]}
loop for each item
Fn->>U: Build Unstructured
note right of U: Preserve metadata.name<br/>and metadata.namespace if present
end
Fn-->>Fn: Return []Unstructured
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks for driving the v2 migration. Two quick confirmations:
Pre-merge checks❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (12)
🧰 Additional context used📓 Path-based instructions (2)**/*.go⚙️ CodeRabbit configuration file
Files:
**/*_test.go⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)request/request_test.go (2)
🔇 Additional comments (13)
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 |
Signed-off-by: Jared Watts <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to build a quick test function that uses the updates in this branch: https://github.com/jbw976/function-v2-test
That function just does the very basic action of returning a variable number of desired ConfigMap
resources (which are namespaced). I was able to build and test this function in a live cluster and verify the expected resources are created when the function runs.
The changes in this PR look good to me! Thanks again for your initiative and effort @twobiers and @bakito @haarchri for also helping test 🙇♂️
Description of your changes
Fixes #225
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Running unit tests