Update limitador to 0.9.0 #608
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #608 +/- ##
==========================================
+ Coverage 80.20% 82.89% +2.68%
==========================================
Files 64 77 +13
Lines 4492 6023 +1531
==========================================
+ Hits 3603 4993 +1390
- Misses 600 681 +81
- Partials 289 349 +60
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
There are few things here that we deliberately left out... e.g. tracing. We purposefully decided to not have them part of the API. This is fairly big change and I think we should be cautious about adding all these. It would also possibly mean needing to support these moving forward! |
removing tracing. Anything else? |
|
Honestly, anything around the cached_redis makes me uncomfortable... |
cached_redis option was already there. This PR adds response-timeout extra option available to the Kuadrant CRD. we should make a call. We either allow all available params for redis_cached or remove |
eguzki
force-pushed
the
update-limitador
branch
4 times, most recently
from
312603b
to
bab68a7
Compare
on parallel run, multiple kuadrant CR were create at the same time
When kuadrant CR object defined limitador replica 1, limitador obj was not updated
|
ready for review @Kuadrant/engineering |
| func LimitadorVerbosityMutator(desired, existing *limitadorv1alpha1.Limitador) bool { | ||
| update := false | ||
| if !reflect.DeepEqual(existing.Spec.Verbosity, desired.Spec.Verbosity) { | ||
| existing.Spec.Verbosity = desired.Spec.Verbosity | ||
| update = true | ||
| } | ||
| return update | ||
| } |
There was a problem hiding this comment.
Do you think it's possible to DRY these funcs a bit, with something like...
func UpdateFieldIfNotEqual(existing, desired interface{}, fieldName string) bool {
existingVal := reflect.ValueOf(existing).Elem().FieldByName(fieldName)
desiredVal := reflect.ValueOf(desired).Elem().FieldByName(fieldName)
if existingVal.IsValid() && desiredVal.IsValid() {
if !existingVal.Equal(desiredVal) {
existingVal.Set(desiredVal)
return true
}
}
return false
}Since we are updating fields when are not matching the desired state, then we can populate the fields in an array and call it either in a for loop or reducer fn like...
limitadorSpecFieldNames := make(string, 0)
limitadorSpecFieldNames = append(limitadorSpecFieldNames,"Affinity")
....
for _, fieldName := range limitadorSpecFieldNames {
if UpdateFieldIfNotEqual(existing, desired, fieldName) {
update = true
}
}not sure if we can even use the limitador spec types for the iteration instead of strings and interface{}
Maybe worth to try... maybe not
There was a problem hiding this comment.
It seems a great idea to avoid DRY. However, I have few little issues with the proposed implementation
- What is the semantics of that
Equalmethod? Each field may need different comparing method. - The decision call of which fields needs to be "updated if not equal", belongs to the caller, not to the mutator.
- TBH: I am not very fan of the reflection lib, only when strictly needed.
| } | ||
| if kObj.Spec.Limitador.Verbosity != nil { | ||
| limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorVerbosityMutator) | ||
| if kObj.Spec.Limitador != nil { |
There was a problem hiding this comment.
If you feel the DRYing is worthy, maybe the reflect lib can help with iteration...
if kObj.Spec.Limitador != nil {
fields := reflect.ValueOf(kObj.Spec.Limitador)
for i := 0; i < v.NumField(); i++ {
// here check if the value `v.Field(i).Interface()` is nil and call the mutator with the desired and expected
}
|
It looks good, maybe there's a pattern for crafting the mutators other than the suggested, we could put a pin and look into it later on too. |
What
Update Limitador to
v0.9.0Bring to the Kuadrant CRD Limtiador's new features:
tracing-> purposefully left out, for now.Added some integrations test to increase coverage.
Refactored reconciliation logic to enforce limitador CR configuration only when defined in the Kuadrant CR object. If not defined in the kuadrant CR object, the controller will not revert changes in the limitador object.