-
Notifications
You must be signed in to change notification settings - Fork 618
feat: add complete request reply data plane #8699
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
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8699 +/- ##
==========================================
- Coverage 51.58% 51.47% -0.12%
==========================================
Files 401 403 +2
Lines 25431 25660 +229
==========================================
+ Hits 13119 13208 +89
- Misses 11509 11638 +129
- Partials 803 814 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Calum Murray <cmurray@redhat.com>
| logger.Warnf("failed to get secret %s in namespace %s: %s", secretName.(string), namespace, err.Error()) | ||
| continue | ||
| } | ||
| replyId, name, namespace, secretName, podIdx, replicaCount := args[0].(string), args[1].(string), args[2].(string), args[3].(string), args[4].(int32), args[5].(int32) |
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.
The comment above says correlationId. In your gist/example I saw both in the response: ce-replyid and ce-correlationid.
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.
Good point - it is worth clarifying this. The idea is that users will set the correlationid for all cloudevents associated with a request, and only set the replyid on the final event for the request (the one that should be treated as the "reply")
I think I was calling it correlationid here since technically this CESQL is generic, and at runtime this variable has the value of the extension attribute. Happy to align to either all correlationid or all replyid
| - eventing.knative.dev | ||
| resources: | ||
| - requestreplies | ||
| - requestreplies/status |
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.
should we add a separate one for finalizers and patch/update - like other roles?
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 don't think so? We only need this for the data plane to be able to see which resources exist/we should be handling - we don't touch the finalizers in this code afaik
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
/test upgrade-tests |
Co-authored-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
/cc @creydr |
|
/test reconciler-tests |
creydr
left a comment
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.
/lgtm
This adds the request reply data plane and associated k8s config to run it. The control plane is responsible for:
Note: TLS/Authn/Authz/observability are out of scope for this first pass implementation. I will work to add these as follow ups
Proposed Changes
main.gofile to be the data plane entry pointPre-review Checklist
Release Note
Docs