-
Notifications
You must be signed in to change notification settings - Fork 438
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
K8s CRD definition for Sentinel rules #182
Conversation
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
=======================================
Coverage 44.51% 44.51%
=======================================
Files 78 78
Lines 4315 4315
=======================================
Hits 1921 1921
Misses 2181 2181
Partials 213 213 Continue to review full report at Codecov.
|
apiVersion: apiextensions.k8s.io/v1 | ||
kind: CustomResourceDefinition | ||
metadata: | ||
name: cbrules.datasource.sentinel.com |
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 abbr might not be clear enough. Maybe cbrules -> circuitbreaker.rules
or circuitbreakerrules
or other better representation?
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.
In open-source world we'd like *.sentinel.io
(though the domain is not owned by us :(
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 prefer circuitbreaker.rules
minimum: 0 | ||
required: | ||
- resource | ||
- limitOrigin |
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.
Only resource, metricType and count are required. For other attributes, we could provide default values.
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.
make sense
- controlBehavior | ||
- paramIndex | ||
- threshold | ||
- maxQueueingTimeMs |
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.
Please also simplify the required list here.
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.
Okay
Duplicated #242 |
Describe what this PR does / why we need it
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews