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

fix: issue when GitHub organization contains more than 30 repos #5746

Merged
merged 7 commits into from
May 20, 2024

Conversation

KoblerS
Copy link
Contributor

@KoblerS KoblerS commented Apr 25, 2024

Fixed issue when GitHub organization contains more than 30 repositories. Scaler will now iterate over the pages according to GitHubs documentation.

Checklist

Fixes #5738

@KoblerS KoblerS requested a review from a team as a code owner April 25, 2024 11:08
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.

Nice improvement! Could you include a unit test for this case too?

pkg/scalers/github_runner_scaler.go Outdated Show resolved Hide resolved
@KoblerS
Copy link
Contributor Author

KoblerS commented Apr 28, 2024

@JorTurFer sure will work on it!

@KoblerS KoblerS force-pushed the feature/fixgithubscaler branch 2 times, most recently from 877071d to bb1d675 Compare May 13, 2024 12:48
@KoblerS KoblerS requested a review from JorTurFer May 13, 2024 12:50
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.

@KoblerS could you please fix the DCO problem?

@KoblerS
Copy link
Contributor Author

KoblerS commented May 17, 2024

@KoblerS could you please fix the DCO problem?

could you please check again. Thanks!

@KoblerS KoblerS requested a review from zroubalik May 17, 2024 09:02
Copy link

semgrep-app bot commented May 17, 2024

Semgrep found 1 context-todo finding:

  • pkg/scalers/github_runner_scaler_test.go

Consider to use well-defined context

Ignore this finding from context-todo.

@KoblerS KoblerS force-pushed the feature/fixgithubscaler branch 6 times, most recently from 41618ff to 72b0d88 Compare May 17, 2024 09:32
KoblerS and others added 4 commits May 17, 2024 11:33
Signed-off-by: Simon Kobler <github@kobler.me>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Simon Kobler <32038731+KoblerS@users.noreply.github.com>
Signed-off-by: Simon Kobler <github@kobler.me>
Signed-off-by: Simon Kobler <github@kobler.me>
Signed-off-by: Simon Kobler <github@kobler.me>
Signed-off-by: Simon Kobler <32038731+KoblerS@users.noreply.github.com>
CHANGELOG.md Outdated Show resolved Hide resolved
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.

There are also some problems in the imports:

@@ -2,13 +2,13 @@ package scalers
 
 import (
 	"context"
+	"crypto/rand"
 	"encoding/json"
 	"fmt"
-	"crypto/rand"
+	"html/template"
 	"math/big"
 	"net/http"
 	"net/http/httptest"
-	"html/template"
 	"strings"
 	"testing"
 	"time"

Signed-off-by: Simon Kobler <github@kobler.me>
@KoblerS KoblerS requested a review from zroubalik May 17, 2024 10:56
@zroubalik
Copy link
Member

@KoblerS you can see the failed static check output: https://github.com/kedacore/keda/actions/runs/9127598269/job/25098204457?pr=5746

Signed-off-by: Simon Kobler <github@kobler.me>
@KoblerS
Copy link
Contributor Author

KoblerS commented May 17, 2024

@KoblerS you can see the failed static check output: https://github.com/kedacore/keda/actions/runs/9127598269/job/25098204457?pr=5746

sorry didn't saw it, should be fixed now.

@zroubalik
Copy link
Member

zroubalik commented May 17, 2024

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

@KoblerS
Copy link
Contributor Author

KoblerS commented May 20, 2024

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

Any updates when this will be merged?

@zroubalik zroubalik merged commit 6aade8f into kedacore:main May 20, 2024
20 checks passed
@KoblerS KoblerS deleted the feature/fixgithubscaler branch May 20, 2024 10:29
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.

GitHub-Runner scaler discovering only first repo list page
3 participants