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.

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

GitHub Runner passive scaler #4281

Merged
merged 25 commits into from
Mar 9, 2023
Merged

GitHub Runner passive scaler #4281

merged 25 commits into from
Mar 9, 2023

Conversation

Eldarrin
Copy link
Contributor

@Eldarrin Eldarrin commented Feb 28, 2023

This is a new scaler that will use GitHub's REST API to schedule scaled runners. It is designed to work in conjunction with GitHub Actions.

Checklist

Fixes #

Relates to #1732

@Eldarrin Eldarrin requested a review from a team as a code owner February 28, 2023 15:34
@semgrep-app
Copy link

semgrep-app bot commented Feb 28, 2023

Semgrep found 6 questionable-assignment findings:

Should meta be modified when an error could be returned?

Created by questionable-assignment.

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.

@Eldarrin the unit test is really great and thorough, though do you think that we can add e2e test as well? It's one of our requirements for a new scaler.

@Eldarrin
Copy link
Contributor Author

Eldarrin commented Mar 5, 2023

I'll look into the e2e next week :)

@zroubalik
Copy link
Member

I'll look into the e2e next week :)

Thanks 🙏 just FYI, we plan to release 2.10 later this week (in case you would want to have that scaler in).

@zroubalik
Copy link
Member

zroubalik commented Mar 8, 2023

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

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.

@Eldarrin could you please fix the Static Checks and please also make sure that you run go mod vendor and include changes in vendor dir in the PR :)

CHANGELOG.md Show resolved Hide resolved
@JorTurFer
Copy link
Member

I have added these envs @Eldarrin :

  • GH_OWNER
  • GH_REPOS
  • GH_SCOPE
  • GH_SO_WORKFLOW_ID
  • GH_WORKFLOW_ID
  • GH_AUTOMATIONS_PAT

Eldarrin and others added 2 commits March 8, 2023 17:55
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

/run-e2e github*

@zroubalik
Copy link
Member

zroubalik commented Mar 8, 2023

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

go.mod Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

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

@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

@Eldarrin , how are you adding the deps to the vendor folder? I ask because there has been missing vendor deps twice and I'm afraid about if you add them manually. To update vendor folder, you just need to execute go mod vendor. Is this what you are doing?

@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

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

@Eldarrin
Copy link
Contributor Author

Eldarrin commented Mar 8, 2023

@Eldarrin , how are you adding the deps to the vendor folder? I ask because there has been missing vendor deps twice and I'm afraid about if you add them manually. To update vendor folder, you just need to execute go mod vendor. Is this what you are doing?

They were in vendor; I just didn’t add them to my commit by mistake. In bed atm watching the builds and getting up when I see another oh damn moment. :)

@JorTurFer
Copy link
Member

@Eldarrin , how are you adding the deps to the vendor folder? I ask because there has been missing vendor deps twice and I'm afraid about if you add them manually. To update vendor folder, you just need to execute go mod vendor. Is this what you are doing?

They were in vendor; I just didn’t add them to my commit by mistake. In bed atm watching the builds and getting up when I see another oh damn moment. :)

No worries, I just asked because it wasn't obvious to me the first time :)

@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

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

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM! 2 small things, only inline and the other:
Could you add a test like https://github.com/kedacore/keda/blob/main/pkg/scalers/artemis_scaler_test.go#L145 to ensure the generated metric name?

pkg/scalers/github_runner_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

You have done an awesome work!
Thanks for this improvement ❤️

@JorTurFer
Copy link
Member

JorTurFer commented Mar 9, 2023

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

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

awesome job! Thanks for the contribution.

@zroubalik zroubalik merged commit 6c0a818 into kedacore:main Mar 9, 2023
xoanmm pushed a commit to xoanmm/keda that referenced this pull request Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants