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

[v2] Added Azure Log Analytics scaler #1085

Merged
merged 12 commits into from
Sep 10, 2020
Merged

[v2] Added Azure Log Analytics scaler #1085

merged 12 commits into from
Sep 10, 2020

Conversation

spoplavskiy
Copy link
Contributor

@spoplavskiy spoplavskiy commented Sep 4, 2020 •
edited
Loading

Checklist

This Scaler related to Feature Request 1061
It implements all features and use-cases described there.

Signed-off-by: Sergiy Poplavskyi spopla@microsoft.com

@spoplavskiy
Added Azure Log Analytics scaler
fd28b0a 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy spoplavskiy mentioned this pull request Sep 4, 2020
Closed
@spoplavskiy
Fixed typos in tests, changed Log Analytics scale position in scale_h…
0aac5f7 
…andler.go, so it in alphabetical order now

Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy spoplavskiy changed the title Added Azure Log Analytics scaler [v2] Added Azure Log Analytics scaler Sep 4, 2020
@tomkerkhove
Copy link
Member

Can you update https://github.com/kedacore/keda/blob/v2/CHANGELOG.md as well please?

@spoplavskiy
Added Azure Log Analytics scaler in changelog
d6355c3 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Copy link
Contributor Author

Can you update https://github.com/kedacore/keda/blob/v2/CHANGELOG.md as well please?

@tomkerkhove , thanks for pointing, added in last commit

@spoplavskiy spoplavskiy closed this Sep 7, 2020
@spoplavskiy spoplavskiy reopened this Sep 7, 2020
zroubalik
zroubalik reviewed Sep 7, 2020
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.

@spoplavskiy thanks for the contribution! I don't feel confident enough to review Azure related scalers, will keep it for the others. But I wonder whether it is doable to add e2e test for this type of scaler?

https://github.com/kedacore/keda/tree/v2/tests

@spoplavskiy
Copy link
Contributor Author

@spoplavskiy thanks for the contribution! I don't feel confident enough to review Azure related scalers, will keep it for the others. But I wonder whether it is doable to add e2e test for this type of scaler?

https://github.com/kedacore/keda/tree/v2/tests

@zroubalik , yes, I can try. The thing is, that Log Analytics workspace should be created on Azure subscription, used by your build pipeline. Also, a Service Principal should have access to this workspace. I see in ".env", that there is variables, related to Service Principal. Can I introduce "TEST_LOG_ANALYTICS_WORKSPACE_ID" variable? I will commit empty value (same, as others), but it should be created and added in Build Pipeline.

@zroubalik
Copy link
Member

Great! I think that @ahmelsayed or @jeffhollan could help you with that

@spoplavskiy
Added E2E tests, changed scaler to support 'FromEnv'
4e60f4a 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Copy link
Contributor Author

Great! I think that @ahmelsayed or @jeffhollan could help you with that

@zroubalik , I have added E2E tests from Log Analytics scaler. Also, added support for 'FromEnv' definitions. I had a conversation with @ahmelsayed, provided him all required scripts to create LA workspace and grant access to Service Principal on subscription, used for tests.

@tomkerkhove
Copy link
Member

Great! I think that @ahmelsayed or @jeffhollan could help you with that

@zroubalik , I have added E2E tests from Log Analytics scaler. Also, added support for 'FromEnv' definitions. I had a conversation with @ahmelsayed, provided him all required scripts to create LA workspace and grant access to Service Principal on subscription, used for tests.

Thanks @spoplavskiy!

@zroubalik
Copy link
Member

@spoplavskiy this is great, e2e tests are really important, thanks! I'll keep the final review on @ahmelsayed

Just a minor nit, could you please rebase the PR, there's a conflict in the Changelog.

@spoplavskiy
Merge branch 'v2' into v2
caac5ef
@spoplavskiy
Fixed static check issues
a0f4c1d 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Fix static check issues
d869bd3
@spoplavskiy
Minor changes: changed messages in logs, query in test
57de6b8 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Copy link
Contributor Author

spoplavskiy commented Sep 9, 2020 •
edited
Loading

@spoplavskiy this is great, e2e tests are really important, thanks! I'll keep the final review on @ahmelsayed

Just a minor nit, could you please rebase the PR, there's a conflict in the Changelog.

@zroubalik , done.

@spoplavskiy
Revert to commit caac5ef
1837c99 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Fix static check issues, minor changes in texts
797496a 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
zroubalik
zroubalik requested changes Sep 9, 2020
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.

Minor nits on logging

@spoplavskiy
Improved logging according to reccomendations
f051d8e 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Copy link
Contributor Author

Minor nits on logging

@zroubalik , thanks for pointing. All your suggestions has been implemented in last commit.

@ahmelsayed
Copy link
Contributor

I added the required secret to the pipeline. Just have a couple of minor comments on the PR.

@spoplavskiy
Improvements, adviced by @ahmelsayed
dc10c11 
Signed-off-by: Sergiy Poplavskyi <spopla@microsoft.com>
@spoplavskiy
Copy link
Contributor Author

I added the required secret to the pipeline. Just have a couple of minor comments on the PR.

@ahmelsayed , thanks!

@ahmelsayed ahmelsayed merged commit 69eae37 into kedacore:v2 Sep 10, 2020
@ahmelsayed ahmelsayed mentioned this pull request Sep 10, 2020
Merged
1 task
@spoplavskiy spoplavskiy mentioned this pull request Sep 10, 2020
Merged
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
@JorTurFer
fix(aws-cloudwatch): Document optional fields (kedacore#1085)
76931e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmelsayed ahmelsayed ahmelsayed approved these changes

@zroubalik zroubalik Awaiting requested review from zroubalik

Assignees

@ahmelsayed ahmelsayed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spoplavskiy @tomkerkhove @zroubalik @ahmelsayed