Use fixed strings instead of knativeAutoscalingPrefix#311
Use fixed strings instead of knativeAutoscalingPrefix#311benjaminhuo merged 1 commit intoOpenFunction:mainfrom
knativeAutoscalingPrefix#311Conversation
| minScale = v | ||
| } | ||
| if _, exist := s.Spec.Annotations[fmt.Sprintf("%s/%s", knativeAutoscalingPrefix, k)]; !exist { | ||
| if _, exist := s.Spec.Annotations[fmt.Sprintf("autoscaling.knative.dev/%s", k)]; !exist { |
There was a problem hiding this comment.
I mean we should put autoscaling.knative.dev/min-scale: 3 in scaleoption.knative, not min-scale: 3 to be consistent with knative.
The other options are similar.
so might be something like below?:
switch k {
case "autoscaling.knative.dev/max-scale", "autoscaling.knative.dev/maxScale":
maxScale = v
case "autoscaling.knative.dev/min-scale", "autoscaling.knative.dev/minScale":
minScale = v
}There was a problem hiding this comment.
are they too long as non-annotation form configurations...
There was a problem hiding this comment.
simply copying knative annotation options here without translation might be better?
There was a problem hiding this comment.
this does reduce the comprehension cost for the user, while it duplicates the effects of spec.serving.annotations
| } | ||
| if _, exist := s.Spec.Annotations[fmt.Sprintf("%s/%s", knativeAutoscalingPrefix, k)]; !exist { | ||
| if _, exist := s.Spec.Annotations[fmt.Sprintf("autoscaling.knative.dev/%s", k)]; !exist { | ||
| s.Spec.Annotations[k] = v |
There was a problem hiding this comment.
I think we can use the same key as the original, because below code is actually not working because it's lack of autoscaling.knative.dev/ prefix
s.Spec.Annotations[k] = vThere was a problem hiding this comment.
And the priority can be adjusted from
// Handle the scale options, which have the following priority relationship:
// Annotations["autoscaling.knative.dev/max-scale" ("autoscaling.knative.dev/min-scale")] >
// ScaleOptions.Knative["max-scale" ("min-scale")] >
// ScaleOptions.maxScale (minScale) >
to
// Handle the scale options, which have the following priority relationship:
// ScaleOptions.Knative["autoscaling.knative.dev/max-scale" ("autoscaling.knative.dev/min-scale")] >
// ScaleOptions.maxScale (minScale) >
// Annotations["autoscaling.knative.dev/max-scale" ("autoscaling.knative.dev/min-scale")] >
which means the "autoscaling.knative.dev/xx in s.spec.annotations has the lowest priority
Use "autoscaling.knative.dev" instead of the const "knativeAutoscalingPrefix". Signed-off-by: laminar <[email protected]>
|
Thanks for the fix! |
Use "autoscaling.knative.dev" instead of the const "knativeAutoscalingPrefix".
Signed-off-by: laminar [email protected]