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.

Sign up for GitHub

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

Jump to bottom

Metrics API Scaler support different formats #5347

Merged
merged 15 commits into from
Mar 24, 2024
Merged

Metrics API Scaler support different formats #5347

merged 15 commits into from
Mar 24, 2024

Conversation

fira42073
Copy link
Contributor

@fira42073 fira42073 commented Jan 3, 2024 •
edited
Loading

Provide a description of what has been changed

Checklist

Fixes #2633

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2024

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

While you are waiting, make sure to:

Learn more about:

@fira42073 fira42073 mentioned this pull request Jan 5, 2024
Closed
@zroubalik
Copy link
Member

Do we have any update on this please?

@fira42073 fira42073 force-pushed the metric-scaler-format branch from a951086 to d8a1406 Compare February 14, 2024 12:09
@fira42073
Copy link
Contributor Author

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 fira42073 force-pushed the metric-scaler-format branch from 4800a4b to 724b63e Compare February 14, 2024 12:33
@fira42073 fira42073 marked this pull request as ready for review February 14, 2024 12:36
@fira42073 fira42073 requested a review from a team as a code owner February 14, 2024 12:36
@fira42073
Copy link
Contributor Author

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 zroubalik requested a review from wozniakjan February 14, 2024 15:28
zroubalik
zroubalik reviewed Feb 14, 2024
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

fira42073 commented Feb 15, 2024 •
edited
Loading

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

Friedrich Albert Kyuri added 5 commits February 29, 2024 13:48
@fira42073
Metrics API Scaler support different format
951d624 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
fix golangci-lint issues
2cba99c 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
trigger ci and go mod tidy
fe853ae 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
gci
6bb1d05 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
fix tests for metrics_api_scaler; add new tests for value_by_path; fi…
c59af14 
…x []interface edge case for value_by_path

Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073 fira42073 force-pushed the metric-scaler-format branch from 611a92b to c59af14 Compare February 29, 2024 12:48
@fira42073 fira42073 requested a review from zroubalik February 29, 2024 16:19
Friedrich Albert Kyuri and others added 3 commits February 29, 2024 17:52
fix: add format field to e2e tests; edit comment about apiformat
b8db664 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
update changelog
9177990 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
Merge branch 'kedacore:main' into metric-scaler-format
2bd4b61
@zroubalik
Copy link
Member

zroubalik commented Mar 1, 2024 •
edited by github-actions bot
Loading

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

@fira42073
Copy link
Contributor Author

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

@fira42073
Copy link
Contributor Author

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

@fira42073
Copy link
Contributor Author

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
Merge branch 'main' into metric-scaler-format
0da2fff 
Signed-off-by: Murr Kyuri <39532283+Friedrich42@users.noreply.github.com>
wozniakjan
wozniakjan reviewed Mar 21, 2024
View reviewed changes
pkg/scalers/metrics_api_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/metrics_api_scaler.go Outdated Show resolved Hide resolved
pkg/util/value_by_path.go Show resolved Hide resolved
pkg/scalers/metrics_api_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/metrics_api_scaler_test.go Outdated Show resolved Hide resolved
pkg/scalers/metrics_api_scaler_test.go Outdated Show resolved Hide resolved
tests/scalers/metrics_api/metrics_api_test.go Outdated Show resolved Hide resolved
@fira42073 @wozniakjan
Apply suggestions from code review
cb37f8f 
Co-authored-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: Murr Kyuri <39532283+Friedrich42@users.noreply.github.com>
fira42073 and others added 2 commits March 21, 2024 14:45
@fira42073
Merge branch 'main' into metric-scaler-format
cc9e66b 
Signed-off-by: Murr Kyuri <39532283+Friedrich42@users.noreply.github.com>
decrease duplication by defining a variable for inputs, revert test f…
b5c15fe 
…ormat field to ensure backwards compatibility

Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
@fira42073
Copy link
Contributor Author

@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?

@fira42073 fira42073 requested a review from wozniakjan March 21, 2024 19:30
@wozniakjan
Copy link
Member

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
Copy link
Contributor Author

fira42073 commented Mar 22, 2024 •
edited
Loading

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

add docstring with examples
177fd27 
Signed-off-by: Friedrich Albert Kyuri <friedrichak42@gmail.com>
wozniakjan
wozniakjan approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fira42073
Copy link
Contributor Author

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

Thanks!

@JorTurFer
Copy link
Member

JorTurFer commented Mar 24, 2024 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer enabled auto-merge (squash) March 24, 2024 22:31
@JorTurFer JorTurFer merged commit 76c6ac0 into kedacore:main Mar 24, 2024
20 checks passed
@fira42073 fira42073 deleted the metric-scaler-format branch March 25, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@wozniakjan wozniakjan wozniakjan approved these changes

@zroubalik zroubalik Awaiting requested review from zroubalik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metrics API Scaler support different format
4 participants
@fira42073 @zroubalik @wozniakjan @JorTurFer