-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat: preliminary dapr runtime support #1518
feat: preliminary dapr runtime support #1518
Conversation
Codecov ReportBase: 62.86% // Head: 63.56% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1518 +/- ##
==========================================
+ Coverage 62.86% 63.56% +0.70%
==========================================
Files 74 75 +1
Lines 10793 10818 +25
==========================================
+ Hits 6785 6877 +92
+ Misses 3447 3376 -71
- Partials 561 565 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Overall this lgtm - what's holding it in draft?
knative/deployer.go
Outdated
aa = make(map[string]string) | ||
|
||
// Static default: Dapr support | ||
aa["dapr.io/enabled"] = "true" |
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 these be constants, eventually?
I'd perhaps also populate all dapr related annotations in a separated call 🤷♂️
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.
+1, the same way we have constants for labels: https://github.com/knative/func/tree/main/k8s/labels
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.
also do we want to apply Dapr annotations to all functions by default?
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 the reviews, please see the latest for:
- labels defined in
labels.go
- Annotations are now extracted and added in the same manner as the functions' annotation map below (looping over the map keys) for consistency
- the annotations define how Dapr can connect its control plane to the service if it is installed, otherwise has no effect
3927c63
to
92f9cce
Compare
Implemented some basic integration tests ensuring things are working as expected. That led to two new PRs improving the client library API for clarity of those tests. The first of those is currently open for review: #1529 PTAL |
- installs Dapr cli in CI - installs Dapr runtime on allocation of test cluster - annotates services to enable dapr sidecar integration - installs redis via helm, enabling state store, pub/sub and distributed lock - integration test added for local invocation - integration test added for service-to-service invocation via the sidecar Note that Dapr runs metrics on port 9002 so as not to collide with Knative metrics.
815d4a3
to
d674daa
Compare
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 awesome work @lkingland ! great to have some integration tests for this, we can expand that later
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland, salaboy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Dapr runtime support - installs Dapr cli in CI - installs Dapr runtime on allocation of test cluster - annotates services to enable dapr sidecar integration - installs redis via helm, enabling state store, pub/sub and distributed lock - integration test added for local invocation - integration test added for service-to-service invocation via the sidecar Note that Dapr runs metrics on port 9002 so as not to collide with Knative metrics. * create constants for knative service labels * extract dapr annotations and use labels
Changes
Adds support for the Dapr runtime from within Functions. Note that the control plane must be installed in the Kubernetes cluster targeted for deployment:
This PR also integrates installation of the Dapr control plane, along with a Redis-backed pub/sub and persistence store, to test/local clusters via
./hack/allocate.sh
./kind enhancement
Release Note
Docs