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

add Azure Pipelines Scaler #1706

Merged
merged 10 commits into from
Apr 14, 2021
Merged

add Azure Pipelines Scaler #1706

merged 10 commits into from
Apr 14, 2021

Conversation

troydn
Copy link
Contributor

@troydn troydn commented Mar 30, 2021 •
edited
Loading

Add a new Azure Pipelines Scaler

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
    - [ ] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable) (add Azure Pipelines Scaler doc keda-docs#411)
  • Changelog has been updated

Relates to #1705

@tomkerkhove
Copy link
Member

Thanks a ton @troydn, this is great!

zroubalik
zroubalik reviewed Apr 6, 2021
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, minor comment!

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
@troydn troydn requested a review from zroubalik April 8, 2021 17:35
zroubalik
zroubalik approved these changes Apr 9, 2021
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, thanks @troydn

Would be nice if you could add e2e test for this scaler.
https://github.com/kedacore/keda/tree/main/tests

@troydn
Copy link
Contributor Author

troydn commented Apr 9, 2021

LGTM, thanks @troydn

Would be nice if you could add e2e test for this scaler.
https://github.com/kedacore/keda/tree/main/tests

I can add e2e tests for this scaler but this would require an Azure DevOps organization that can be used for the e2e tests. Should I create one and provide the details/access or would you rather provide one?

@tomkerkhove
Copy link
Member

I can set one up, what do you need? Org id and PAT?

@tomkerkhove
Copy link
Member

And what PAT permissions?

@zroubalik
Copy link
Member

@tomkerkhove great, just to be sure, we will need something pernament once the tests are ready. Mabye @jeffhollan or @ahmelsayed could help with that?

@tomkerkhove
Copy link
Member

They are free so should be OK!

@tomkerkhove
Copy link
Member

tomkerkhove commented Apr 9, 2021 •
edited
Loading

Org created as per kedacore/governance#20 and is up on https://dev.azure.com/kubernetes-event-driven-autoscaling/Kubernetes%20Event-driven%20Autoscaling%20(KEDA)

@troydn
Copy link
Contributor Author

troydn commented Apr 10, 2021

Org created as per kedacore/governance#20 and is up on https://dev.azure.com/kubernetes-event-driven-autoscaling/Kubernetes%20Event-driven%20Autoscaling%20(KEDA)

Thanks!
Should the e2e test also manage the creation of the build definition and the agent pool on Azure DevOps? It could also be a one time setup where an empty agent pool is created and a simple build definition is created that sleeps for 2 minutes.

@tomkerkhove
Copy link
Member

I'm happy to set it up, I'll create an empty pool and pipeline that has a 2 min delay in it.

What else do you need in terms of config and what PAT permissions are required?

@troydn
Copy link
Contributor Author

troydn commented Apr 12, 2021

I'm happy to set it up, I'll create an empty pool and pipeline that has a 2 min delay in it.

What else do you need in terms of config and what PAT permissions are required?

I finished the e2e test. But I'm not sure what image should be used for the Azure DevOps Agent. For now, I used one on my docker registry. But I think this should be replaced by one on a registry managed by KEDA.

The test requires an Azure DevOps environment with the following config:

  • Agent pool created
  • A pipeline configured to run on the created agent pool with "sleep 2m" as build step
  • A PAT with the following permissions:
    • Agent Pools : Read & manage
    • Build: Read & execute

@tomkerkhove
Copy link
Member

I'm happy to set it up, I'll create an empty pool and pipeline that has a 2 min delay in it.
What else do you need in terms of config and what PAT permissions are required?

I finished the e2e test. But I'm not sure what image should be used for the Azure DevOps Agent. For now, I used one on my docker registry. But I think this should be replaced by one on a registry managed by KEDA.

We host all test images on https://github.com/kedacore/test-tools, are you OK in adding a container image there?

The test requires an Azure DevOps environment with the following config:

  • Agent pool created

  • A pipeline configured to run on the created agent pool with "sleep 2m" as build step

  • A PAT with the following permissions:

    • Agent Pools : Read & manage
    • Build: Read & execute

I'll get this setup tomorrow.

@tomkerkhove tomkerkhove mentioned this pull request Apr 13, 2021
Merged
@tomkerkhove
Copy link
Member

tomkerkhove commented Apr 13, 2021 •
edited
Loading

@troydn I've added these GitHub secrets for you:
image

Pipeline is up on https://dev.azure.com/kubernetes-event-driven-autoscaling/Kubernetes%20Event-driven%20Autoscaling%20(KEDA)/_build?definitionId=1 with PR open on kedacore/test-tools#11

Feel free to let me know if you're missing something.

@troydn troydn mentioned this pull request Apr 13, 2021
Merged
@troydn
Copy link
Contributor Author

troydn commented Apr 14, 2021

@tomkerkhove thanks for your help on this. I think the e2e test is ready now.

tomkerkhove
tomkerkhove approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, can you just fix the merge conflict please?

@troydn
add Azure Pipelines Scaler
efb8bd1 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
fix pre-commit errors
2e95692 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
fix testdata
54b2fa6 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
add organization name to metricName
e1170d7 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
fix unit test
4735a0c 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
add e2e test
4d3343f 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
fix trailing whitespace
ef64a8e 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
remove whitespace
a646408 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
use image from keda registry
f90adf8 
Signed-off-by: Troy <troydenorme@hotmail.com>
@troydn
add GitHub secrets for e2e test
db92875 
Signed-off-by: Troy <troydenorme@hotmail.com>
@tomkerkhove tomkerkhove merged commit 38b5a7f into kedacore:main Apr 14, 2021
@tomkerkhove
Copy link
Member

Thank you @troydn and let me know if you are interested for the blog post! (kerkhove.tom@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

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees

@ahmelsayed ahmelsayed

@zroubalik zroubalik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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