From 83eb1d1d8dc3109cbcda8a5cca6914d50c6b0db8 Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Wed, 15 May 2024 16:28:05 -0700 Subject: [PATCH 1/6] ok --- .../clientcreatorpool/clientcreatorpool.go | 62 +++++++++++++++++++ .../clientcreatorpool_test.go | 0 2 files changed, 62 insertions(+) create mode 100644 server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go create mode 100644 server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go new file mode 100644 index 000000000..66b3a179e --- /dev/null +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go @@ -0,0 +1,62 @@ +package clientcreatorpool + +import ( + "github.com/palantir/go-githubapp/githubapp" + "github.com/pkg/errors" + ghClient "github.com/runatlantis/atlantis/server/neptune/workflows/activities/github" + "github.com/uber-go/tally/v4" +) + +type ClientCreatorPool struct { + NamesToClientCreators map[string]githubapp.ClientCreator + NamesToRateLimitRemaining map[string]int +} + +type ClientCreatorPoolConfig struct { + name string + config githubapp.Config +} + +func (c *ClientCreatorPool) Initialize(configs []ClientCreatorPoolConfig, scope tally.Scope) error { + + c.NamesToClientCreators = make(map[string]githubapp.ClientCreator) + c.NamesToRateLimitRemaining = make(map[string]int) + + for _, config := range configs { + clientCreator, err := githubapp.NewDefaultCachingClientCreator( + config.config, + githubapp.WithClientMiddleware( + ghClient.ClientMetrics(scope.SubScope("app"+config.name)), + )) + if err != nil { + return errors.Wrap(err, "client creator") + } + c.NamesToClientCreators[config.name] = clientCreator + // just needs to be non-zero, true value will be updated within 60 seconds by the cron that checks the rate limit + c.NamesToRateLimitRemaining[config.name] = 100 + } + + return nil +} + +func (c *ClientCreatorPool) GetClientCreatorWithMaxRemainingRateLimit(name string) (githubapp.ClientCreator, error) { + maxSeenSoFar := 0 + for clientName, num := range c.NamesToRateLimitRemaining { + if num > maxSeenSoFar { + maxSeenSoFar = num + name = clientName + } + } + + clientCreator, ok := c.NamesToClientCreators[name] + if !ok { + return nil, errors.New("client creator not found") + } + + return clientCreator, nil +} + +// this func will be used in the crons to update the rate limit remaining +func (c *ClientCreatorPool) SetRateLimitRemaining(name string, remaining int) { + c.NamesToRateLimitRemaining[name] = remaining +} diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go new file mode 100644 index 000000000..e69de29bb From b9d2ff8182608dc088a14f9cd6b91e5aecc55dee Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Wed, 15 May 2024 18:54:51 -0700 Subject: [PATCH 2/6] ok --- .../clientcreatorpool/clientcreatorpool.go | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go index 66b3a179e..521f9d0f0 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go @@ -1,6 +1,8 @@ package clientcreatorpool import ( + "fmt" + "github.com/palantir/go-githubapp/githubapp" "github.com/pkg/errors" ghClient "github.com/runatlantis/atlantis/server/neptune/workflows/activities/github" @@ -8,47 +10,58 @@ import ( ) type ClientCreatorPool struct { - NamesToClientCreators map[string]githubapp.ClientCreator - NamesToRateLimitRemaining map[string]int + // keys are app ids aka integration ids + // Note: It would make a lot more sense to use something like a name, or the slug, right? + // The reason why that is not the case, is that then we would have to pass those around. We can't + // modify the clientCreator (it is part of the githubapp library), and you can see that inside, its + // fields are private, and there is no way to associate a given clientCreator with a name. There is + // no slug inside the githubapp.Config, only private keys and app ids, and we don't want to use + // private keys as keys. + idToClientCreator map[int]githubapp.ClientCreator + idToRateLimitRemaning map[int]int + // Note that integration id is NOT installation id. Those are 2 separate things. } type ClientCreatorPoolConfig struct { - name string + id int config githubapp.Config } +// This function is only called once, when the server starts func (c *ClientCreatorPool) Initialize(configs []ClientCreatorPoolConfig, scope tally.Scope) error { - - c.NamesToClientCreators = make(map[string]githubapp.ClientCreator) - c.NamesToRateLimitRemaining = make(map[string]int) + c.idToClientCreator = make(map[int]githubapp.ClientCreator) + c.idToRateLimitRemaning = make(map[int]int) for _, config := range configs { + t := fmt.Sprintf("github.app.%d", config.id) clientCreator, err := githubapp.NewDefaultCachingClientCreator( config.config, githubapp.WithClientMiddleware( - ghClient.ClientMetrics(scope.SubScope("app"+config.name)), + // combine the id with app + ghClient.ClientMetrics(scope.SubScope(t)), )) if err != nil { return errors.Wrap(err, "client creator") } - c.NamesToClientCreators[config.name] = clientCreator + c.idToClientCreator[config.id] = clientCreator // just needs to be non-zero, true value will be updated within 60 seconds by the cron that checks the rate limit - c.NamesToRateLimitRemaining[config.name] = 100 + c.idToRateLimitRemaning[config.id] = 100 } return nil } -func (c *ClientCreatorPool) GetClientCreatorWithMaxRemainingRateLimit(name string) (githubapp.ClientCreator, error) { +func (c *ClientCreatorPool) GetClientCreatorWithMaxRemainingRateLimit() (githubapp.ClientCreator, error) { maxSeenSoFar := 0 - for clientName, num := range c.NamesToRateLimitRemaining { + theId := 0 + for id, num := range c.idToRateLimitRemaning { if num > maxSeenSoFar { maxSeenSoFar = num - name = clientName + theId = id } } - clientCreator, ok := c.NamesToClientCreators[name] + clientCreator, ok := c.idToClientCreator[theId] if !ok { return nil, errors.New("client creator not found") } @@ -57,6 +70,6 @@ func (c *ClientCreatorPool) GetClientCreatorWithMaxRemainingRateLimit(name strin } // this func will be used in the crons to update the rate limit remaining -func (c *ClientCreatorPool) SetRateLimitRemaining(name string, remaining int) { - c.NamesToRateLimitRemaining[name] = remaining +func (c *ClientCreatorPool) SetRateLimitRemaining(id int, remaining int) { + c.idToRateLimitRemaning[id] = remaining } From b968740e6cbc8b2419848b60ff0e70fb9f22776f Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Sat, 18 May 2024 13:47:26 -0700 Subject: [PATCH 3/6] ok --- .../clientcreatorpool/clientcreatorpool.go | 4 + .../clientcreatorpool_test.go | 74 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go index 521f9d0f0..a7788084c 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go @@ -73,3 +73,7 @@ func (c *ClientCreatorPool) GetClientCreatorWithMaxRemainingRateLimit() (githuba func (c *ClientCreatorPool) SetRateLimitRemaining(id int, remaining int) { c.idToRateLimitRemaning[id] = remaining } + +func (c *ClientCreatorPool) GetRateLimitRemaining(id int) int { + return c.idToRateLimitRemaning[id] +} diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go index e69de29bb..d230f883c 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go @@ -0,0 +1,74 @@ +package clientcreatorpool + +import ( + "testing" + + "github.com/palantir/go-githubapp/githubapp" + "github.com/runatlantis/atlantis/server/logging" + "github.com/runatlantis/atlantis/server/metrics" + "github.com/stretchr/testify/assert" +) + +func initialize(t *testing.T) ClientCreatorPool { + configs := []ClientCreatorPoolConfig{ + { + id: 564754, + config: githubapp.Config{}, + }, + { + id: 243643, + config: githubapp.Config{}, + }, + } + + configs[0].config.App.IntegrationID = 564754 + configs[0].config.App.PrivateKey = "key1" + configs[0].config.App.WebhookSecret = "secret1" + + configs[1].config.App.IntegrationID = 243643 + configs[1].config.App.PrivateKey = "key2" + configs[1].config.App.WebhookSecret = "secret2" + + c := ClientCreatorPool{} + ctxLogger := logging.NewNoopCtxLogger(t) + scope, _, _ := metrics.NewLoggingScope(ctxLogger, "null") + c.Initialize(configs, scope) + return c +} + +func TestInitialize(t *testing.T) { + configs := []ClientCreatorPoolConfig{ + { + id: 1, + config: githubapp.Config{}, + }, + { + id: 2, + config: githubapp.Config{}, + }, + } + + configs[0].config.App.IntegrationID = 1 + configs[0].config.App.PrivateKey = "key1" + configs[0].config.App.WebhookSecret = "secret1" + + configs[1].config.App.IntegrationID = 2 + configs[1].config.App.PrivateKey = "key2" + configs[1].config.App.WebhookSecret = "secret2" + + c := ClientCreatorPool{} + ctxLogger := logging.NewNoopCtxLogger(t) + scope, _, _ := metrics.NewLoggingScope(ctxLogger, "null") + err := c.Initialize(configs, scope) + assert.NoError(t, err) +} + +func TestGetClientCreatorWithMaxRemainingRateLimit(t *testing.T) { + c := initialize(t) + c.SetRateLimitRemaining(564754, 9000) + clientCreator, err := c.GetClientCreatorWithMaxRemainingRateLimit() + assert.NoError(t, err) + assert.NotNil(t, clientCreator) + assert.Equal(t, 9000, c.GetRateLimitRemaining(564754)) + +} From e6c5995a4e61b964896b418a29e304a9dccae58a Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Sat, 18 May 2024 13:52:44 -0700 Subject: [PATCH 4/6] ok --- .../temporalworker/clientcreatorpool/clientcreatorpool_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go index d230f883c..d854d7028 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go @@ -32,7 +32,7 @@ func initialize(t *testing.T) ClientCreatorPool { c := ClientCreatorPool{} ctxLogger := logging.NewNoopCtxLogger(t) scope, _, _ := metrics.NewLoggingScope(ctxLogger, "null") - c.Initialize(configs, scope) + _ = c.Initialize(configs, scope) return c } @@ -70,5 +70,4 @@ func TestGetClientCreatorWithMaxRemainingRateLimit(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, clientCreator) assert.Equal(t, 9000, c.GetRateLimitRemaining(564754)) - } From 171a76778b7abdb62692e7e165ebd5a0474b752e Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Sat, 18 May 2024 14:53:54 -0700 Subject: [PATCH 5/6] ok --- .../temporalworker/clientcreatorpool/clientcreatorpool_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go index d854d7028..5e0f529ea 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool_test.go @@ -69,5 +69,6 @@ func TestGetClientCreatorWithMaxRemainingRateLimit(t *testing.T) { clientCreator, err := c.GetClientCreatorWithMaxRemainingRateLimit() assert.NoError(t, err) assert.NotNil(t, clientCreator) + assert.Equal(t, clientCreator, c.idToClientCreator[564754]) assert.Equal(t, 9000, c.GetRateLimitRemaining(564754)) } From 4b5210a3a636505c54719a80bc70f509f99fd56b Mon Sep 17 00:00:00 2001 From: Shawna Monero Date: Mon, 20 May 2024 12:06:42 -0700 Subject: [PATCH 6/6] change comment --- .../temporalworker/clientcreatorpool/clientcreatorpool.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go index a7788084c..1a72dd0ed 100644 --- a/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go +++ b/server/neptune/temporalworker/clientcreatorpool/clientcreatorpool.go @@ -44,7 +44,8 @@ func (c *ClientCreatorPool) Initialize(configs []ClientCreatorPoolConfig, scope return errors.Wrap(err, "client creator") } c.idToClientCreator[config.id] = clientCreator - // just needs to be non-zero, true value will be updated within 60 seconds by the cron that checks the rate limit + // For the rate limit remaining, initially needs to be non-zero, the actual rate limit number value will be + // updated within 60 seconds by the cron that checks the rate limit c.idToRateLimitRemaning[config.id] = 100 }