Use fixed strings instead of knativeAutoscalingPrefix
#311
Conversation
| @@ -256,26 +255,22 @@ func (r *servingRun) createService(s *openfunction.Serving, cm map[string]string | |||
| case "min-scale", "minScale": | |||
| 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
| @@ -256,26 +255,22 @@ func (r *servingRun) createService(s *openfunction.Serving, cm map[string]string | |||
| case "min-scale", "minScale": | |||
| 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 { | |||
| 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 <fangtian@kubesphere.io>
|
Thanks for the fix! |
Use "autoscaling.knative.dev" instead of the const "knativeAutoscalingPrefix".
Signed-off-by: laminar fangtian@kubesphere.io