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

feat: Introduce activationThreshold/minMetricValue for Azure scalers #3383

Merged
merged 18 commits into from
Jul 19, 2022
Merged

feat: Introduce activationThreshold/minMetricValue for Azure scalers #3383

merged 18 commits into from
Jul 19, 2022

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Jul 16, 2022 •
edited
Loading

I have added the test case to every already existing e2e tests except in app-insights. They are quite complex and I need to review it with calm (or maybe wait till anyone migrates it)

All azure e2e test have been migrated to golang and updated to cover this new cases. Azure Monitor and Azure Event Hub haven't been done because there were already existing e2e test.

In Azure Application Inisghts, I had to use goroutines to keep the value alive in order to query it from Azure service, I have tried several things but other options produce flaky tests, this seems more reliable. There were several configurations tested in typescript e2e test, but they are basically the same with different TriggerAuthentication configurations (secrets, envs, aad-pod), due to we don't have aad-pod-identity deployed in our clusters and secrets/envs are quite similar, I have migrated only the e2e that uses secrets

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added

Fixes #3259
Fixes #3228
Related to #2800
Related to kedacore/keda-docs#818

@JorTurFer JorTurFer requested a review from a team as a code owner July 16, 2022 20:47
@JorTurFer JorTurFer mentioned this pull request Jul 16, 2022
Merged
1 task
@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 16, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 16, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 17, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 17, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 17, 2022 •
edited by github-actions bot
Loading

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

@zroubalik zroubalik mentioned this pull request Jul 18, 2022
Merged
2 tasks
@zroubalik zroubalik requested a review from v-shenoy July 18, 2022 10:54
@JorTurFer JorTurFer changed the title feat: Introduce activationThreshold/minMetricValue for Azure scalers WIP feat: Introduce activationThreshold/minMetricValue for Azure scalers Jul 18, 2022
@JorTurFer JorTurFer force-pushed the activation-threshold-azure branch from 33e1826 to 51402f8 Compare July 18, 2022 19:42
@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 18, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer changed the title WIP feat: Introduce activationThreshold/minMetricValue for Azure scalers feat: Introduce activationThreshold/minMetricValue for Azure scalers Jul 19, 2022
@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited
Loading

I have migrated app-insights and az-pipelines e2e. Only Azure Monitor and Azure EventHub left because they didn't have e2e test, do you think that it's worth to add them from scratch in this PR @zroubalik ?

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited
Loading

There is a spelling error related with e2e environment variables . Once this PR is ready to merge, I'll update those variables to correct the error before merging (if I correct them now, e2e test will fail due to missing envs)

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@zroubalik
Copy link
Member

I have migrated app-insights and az-pipelines e2e. Only Azure Monitor and Azure EventHub left because they didn't have e2e test, do you think that it's worth to add them from scratch in this PR @zroubalik ?

I think we can keep it for another PR.

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited
Loading

I think we can keep it for another PR.

This PR is ready to review in that case 😄

zroubalik
zroubalik reviewed Jul 19, 2022
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, just a minor nit, using 1 instead of a is imho more explicit and clear on a quick look, even though it has the same meaning.

tests/scalers_go/azure_application_insights/azure_application_insights_test.go Outdated Show resolved Hide resolved
tests/scalers_go/azure_pipelines/azure_pipelines_test.go Outdated Show resolved Hide resolved
tests/scalers_go/azure_pipelines/azure_pipelines_test.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

Looking good, just a minor nit, using 1 instead of a is imho more explicit and clear on a quick look, even though it has the same meaning.

@JorTurFer could you please also rebase, I think I have fixed bunch of these a -> 1 in a previous PR.

JorTurFer added 4 commits July 19, 2022 12:04
@JorTurFer
Add support to activation in Azure scalers
483e39e 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
add activation e2e in go
bbd5ffa 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
update az pipelines e2e
8d8ae57 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
fix errors
339316c 
Signed-off-by: Jorge Turrado <[email protected]>
JorTurFer and others added 12 commits July 19, 2022 12:04
@JorTurFer
fix errors
8b61d59 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
fix errors
f5a921a 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
fix errors
e1b6adf 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
use unique queues
274f706 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
Add missing chenge
a9ddbe1 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
migrate az pipelines e2e
fa47267 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
migrate app-insights e2e
ba52ea5 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
fix styles
96a267d 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
fix typo
9c467a7 
Signed-off-by: Jorge Turrado <[email protected]>
fix typo and increase test timeout
2393058 
Signed-off-by: Jorge Turrado <[email protected]>
increase test timeout
cf00024 
Signed-off-by: Jorge Turrado <[email protected]>
apply feedback
7a12cdb 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer JorTurFer force-pushed the activation-threshold-azure branch from 29a7c41 to 7a12cdb Compare July 19, 2022 10:07
@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 19, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

@JorTurFer could you please also rebase, I think I have fixed bunch of these a -> 1 in a previous PR.

done the rebase and updated the code :)

zroubalik
zroubalik approved these changes Jul 19, 2022
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.

LGTM, great job!

fix typo
126bf85 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer JorTurFer enabled auto-merge (squash) July 19, 2022 12:25
fix naming
d607a80 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer JorTurFer merged commit 875e7e2 into kedacore:main Jul 19, 2022
@JorTurFer JorTurFer deleted the activation-threshold-azure branch July 19, 2022 12:52
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

@v-shenoy v-shenoy Awaiting requested review from v-shenoy

Assignees
No one assigned
Labels
skip-e2e
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate e2e test for Azure Pipelines to Go Migrate e2e test for Azure Application Insights to Go
2 participants
@JorTurFer @zroubalik