Skip to content
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

Scaling Modifiers formula ambiguous behavior #5074

Closed
danialgood opened this issue Oct 12, 2023 · 6 comments
Closed

Scaling Modifiers formula ambiguous behavior #5074

danialgood opened this issue Oct 12, 2023 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@danialgood
Copy link

danialgood commented Oct 12, 2023

Report

It seems that since expr.AsFloat64() is used while compiling the formula, all possible outputs of the formula should be float.

Currently the examples provided in the documentation regarding ternary conditions are using int values. If having the value always as float is intended (which i guess it is) then doc should be revised to use float values otherwise the code should be changed.

Expected Behavior

examples provided in the docs to work

Actual Behavior

admissions webhook denying validation :

Error from server (Forbidden): error when creating : admission webhook "vscaledobject.kb.io" denied the request: error validating formula in ScalingModifiers expected float64, but got interface {}

Steps to Reproduce the Problem

  1. Add scalingModifiers to ScaledObject specs
  2. Add a formula with ternary condition

Logs from KEDA operator

example

KEDA Version

2.12.0

Kubernetes Version

1.27

Platform

Any

Scaler Details

No response

Anything else?

No response

@danialgood danialgood added the bug Something isn't working label Oct 12, 2023
@zroubalik
Copy link
Member

@gauron99 PTAL 🙏

@gauron99
Copy link
Contributor

hey @danialgood thanks for letting us know! It looks like this is a mistake on my end. Ive updated the docs here

@gauron99
Copy link
Contributor

gauron99 commented Oct 12, 2023

So for now Ive updated the docs. I recognize that having to specify float is perhaps less intuitive then using ints - especially when you can mix the two in different cases.

So an idea is to perhaps convert all necessary ints to floats before compiling to keep the AsFloat() function and keep the user-friendly input as well

I will work on this

@danialgood
Copy link
Author

Great! I also noticed that expr documentation under AsFloat64 explicitly mentions that:

If the expression returns integer value, Expr will convert it to float64.

I didn't check out expr source code but might be an issue on their end.

@gauron99
Copy link
Contributor

Expr package returns any not int on ternary operator therefore its necessary to cast to float before returning in formula. This can be done using the expr package, Ive created a PR in docs here

@JorTurFer
Copy link
Member

As the documentation is already fixed, I close the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

4 participants