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

Azure Application Insights: Add option to ignore null values #4329

Merged
merged 4 commits into from
Mar 8, 2023
Merged

Azure Application Insights: Add option to ignore null values #4329

merged 4 commits into from
Mar 8, 2023

Conversation

yardenshoham
Copy link
Contributor

@yardenshoham yardenshoham commented Mar 7, 2023 •
edited by tomkerkhove
Loading

Added the option to ignore errors during the extraction of Azure Application Insights values

Checklist

  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Relates to #4316

Inspired by #3073
Relates to kedacore/keda-docs#1084

@yardenshoham yardenshoham requested a review from a team as a code owner March 7, 2023 08:59
@yardenshoham
Azure Application Insights: Add option to ignore null values
bab27a1 
Also fix typo

Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
zroubalik
zroubalik reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good. Could you please add a test for this usecase?

@yardenshoham
Add metadata tests
dd53b9a 
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
@yardenshoham
Copy link
Contributor Author

Tests added

@zroubalik
Copy link
Member

zroubalik commented Mar 7, 2023 •
edited by github-actions bot
Loading

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

@yardenshoham
Fix tests
b16ea0d 
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
@yardenshoham
Copy link
Contributor Author

Tests fixed, please rerun e2e tests

@JorTurFer
Copy link
Member

JorTurFer commented Mar 7, 2023 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer 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.

Thanks for the contribution!
Looking good. Only a small nit inline

pkg/scalers/azure_app_insights_scaler.go Outdated Show resolved Hide resolved
@yardenshoham
Change default to false
7727c8f 
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
@yardenshoham
Copy link
Contributor Author

Please run e2e tests

@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023 •
edited by github-actions bot
Loading

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

@tomkerkhove tomkerkhove mentioned this pull request Mar 8, 2023
Merged
1 task
@zroubalik zroubalik merged commit 6d0d60c into kedacore:main Mar 8, 2023
@yardenshoham yardenshoham deleted the ignore branch March 8, 2023 13:32
@yardenshoham yardenshoham mentioned this pull request Mar 8, 2023
Closed
xoanmm pushed a commit to xoanmm/keda that referenced this pull request Mar 22, 2023
@yardenshoham @xoanmm
Azure Application Insights: Add option to ignore null values (kedacor…
65709b3 
…e#4329)

Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@yardenshoham @zroubalik @JorTurFer