Metrics API Scaler support different formats

[fira42073]

Provide a description of what has been changed

Checklist

• When introducing a new scaler, I agree with the scaling governance policy
• I have verified that my change is according to the deprecations & breaking changes policy
• Tests have been added
• Changelog has been updated and is aligned with our changelog requirements
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #2633

[github-actions]

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

• Add an entry in our changelog in alphabetical order and link related issue
• Update the documentation, if needed
• Add unit & e2e tests for your changes
• GitHub checks are passing
• Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

• Our contribution guide

[zroubalik]

Do we have any update on this please?

[fira42073]

It seems like I have messed up the history, because I pulled the changes from kedacore:main and then tried to add DCO. I'll fix it asap

[fira42073]

Hey. Hope you are well. Could you please review my code? I think it's pretty much ready.
If all is good, I'll finish the rest of the checklist and it's ready to be merged.
@zroubalik

[zroubalik]

There are test failures:

Test failures:
  TestGetValueFromResponse: github.com/kedacore/keda/v2/pkg/scalers
  TestGetValueFromResponse/json:_string: github.com/kedacore/keda/v2/pkg/scalers

    metrics_api_scaler_test.go:155: Expected 64, got 64

  TestGetValueFromResponse/json:_{}.[].{}: github.com/kedacore/keda/v2/pkg/scalers

    metrics_api_scaler_test.go:155: Expected 32, got 32

  TestGetValueFromResponse/yaml:_integer: github.com/kedacore/keda/v2/pkg/scalers

    metrics_api_scaler_test.go:151: Expected success but got error: yaml: mapping values are not allowed in this context
    metrics_api_scaler_test.go:155: Expected 2.43, got 0

  TestGetValueFromResponse/yaml:_string: github.com/kedacore/keda/v2/pkg/scalers

    metrics_api_scaler_test.go:151: Expected success but got error: yaml: mapping values are not allowed in this context
    metrics_api_scaler_test.go:155: Expected 64, got 0

  TestGetValueFromResponse/yaml:_{}.[].{}: github.com/kedacore/keda/v2/pkg/scalers

    metrics_api_scaler_test.go:151: Expected success but got error: yaml: mapping values are not allowed in this context
    metrics_api_scaler_test.go:155: Expected 32, got 0

This should be also covered by e2e tests:

https://github.com/kedacore/keda/blob/main/tests/scalers/metrics_api/metrics_api_test.go

Thanks!

[fira42073]

I have fixed the unit tests. Will get e2e done soon too

[zroubalik]

/run-e2e metrics_api
Update: You can check the progress here

[fira42073]

The tests seem to pass. Shall I also document this change in keda-docs?

[fira42073]

Documentation is done as well. You can check it here:
kedacore/keda-docs#1325

[fira42073]

I believe that it can be merged if you don't have any additional comments.
Feel free to commit into this/doc branch as well if you want to fix some small detail or something.
@wozniakjan @zroubalik

[fira42073]

@wozniakjan
Thank you for the review!

I've addressed most of the points.
Using k8s package is a great idea, but it may not be suitable here, because it doesn't support .number notation.

What do you think we should do about that?

[wozniakjan]

Using k8s package is a great idea, but it may not be suitable here, because it doesn't support .number notation.
What do you think we should do about that?

I didn't realize the incompatibility, thanks for the thorough testing and you got me convinced to go with GetValueByPath :)

Now FOSSA tests appear to be failing, let me go through the repo to see if there is any info regarding these

Error:  An issue occurred
  *** Relevant Errors ***
      Error: The build failed. Check the FOSSA webapp for more details
      → src/App/Fossa/API/BuildWait.hs:119:41

[fira42073]

Using k8s package is a great idea, but it may not be suitable here, because it doesn't support .number notation.
What do you think we should do about that?

I didn't realize the incompatibility, thanks for the thorough testing and you got me convinced to go with GetValueByPath :)

Now FOSSA tests appear to be failing, let me go through the repo to see if there is any info regarding these

Error:  An issue occurred
  *** Relevant Errors ***
      Error: The build failed. Check the FOSSA webapp for more details
      → src/App/Fossa/API/BuildWait.hs:119:41

I don't think it's actually failing; just need to restart it; itself report is clean

[wozniakjan]

lgtm
(not sure if approval from me is sufficient, maybe @kedacore/keda-contributors will need to approve as well)

[fira42073]

@zroubalik this one is ready to go.
Feel free to merge it.

Thanks!

[JorTurFer]

/run-e2e metrics
Update: You can check the progress here