[ALI] adds AUTH_GH_ORG and AUTH_GH_REPO to set/restrict authentication and management for a specific org/repo #7152
Conversation
…nstead of guessing
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
bot
added
the
CLA Signed
| @@ -9,6 +9,18 @@ variable "aws_region_instances" { | |||
| type = list(string) | |||
| } | |||
|
|
|||
| variable "auth_gh_app" { | |||
There was a problem hiding this comment.
Why is this var needed? Don't we already get this token somehow?
| default = "" | ||
| } | ||
|
|
||
| variable "auth_gh_org" { |
There was a problem hiding this comment.
naming nit: can we have a name without "auth" in it? Maybe "ci_github_org" or just "github_org"
The auth part is rather incidental here (a gh app can be authorized to affect multiple github orgs). The key thing we want to know is which github org is this particular autoscaler fleet supposed to be managing.
| @@ -53,16 +55,16 @@ resource "aws_lambda_function" "scale_up_chron" { | |||
| MUST_HAVE_ISSUES_LABELS = join(",", var.must_have_issues_labels) | |||
| REDIS_ENDPOINT = var.redis_endpoint | |||
| REDIS_LOGIN = var.redis_login | |||
| RETRY_SCALE_UP_CHRON_RECORD_QUEUE_URL = var.sqs_build_queue_retry.url | |||
There was a problem hiding this comment.
random: were these automated changes via some lint tool? Or did you change it manaully?
Total side comment, but it would be nice if the automation was part of CI!
| @@ -94,9 +94,6 @@ export function getRepo(repoDef: string, repoName?: string): Repo { | |||
| } | |||
|
|
|||
| const repoArr = repoDef.split('/'); | |||
| if (repoArr.length != 2) { | |||
There was a problem hiding this comment.
why is this no longer needed?
| @@ -5,7 +5,7 @@ import { SQSRecord } from 'aws-lambda'; | |||
| import { expBackOff } from './utils'; | |||
| import { Metrics } from './metrics'; | |||
|
|
|||
| function getQueueUrl(evt: SQSRecord, sqs: SQS) { | |||
| function getQueueUrl(evt: SQSRecord) { | |||
There was a problem hiding this comment.
nit: There seem to be a lot of lint changes here. This PR would be way easier to review if split into a "lint only" changes PR and a "new functionality" PR
| /* istanbul ignore next */ | ||
| this.authGHOrg = process.env.AUTH_GH_ORG; | ||
| /* istanbul ignore next */ | ||
| this.authGHRepo = process.env.AUTH_GH_REPO; |
There was a problem hiding this comment.
I don't think this is the env var you're setting in terraform?
| @@ -263,14 +263,21 @@ export function backwardCompatibleGetRepoForgetRunnerTypes(ec2runner: RunnerInfo | |||
| return getRepo(ec2runner.repo as string); | |||
| } | |||
|
|
|||
| export function backwardCompatibleGetAuthRepoForGetRunnerTypes(ec2runner: RunnerInfo): Repo { | |||
There was a problem hiding this comment.
nit on the term "auth" in AuthRepo (see comment on variables.tf)
| @@ -3,7 +3,7 @@ SHELL=/bin/bash -o pipefail | |||
| .PHONY: clean | |||
| clean: | |||
| rm -rf dist node_modules coverage | |||
| rm runners.zip | |||
| rm -rf runners.zip | |||
There was a problem hiding this comment.
I'm surprised this was needed
| const validRunnerTypes = await getRunnerTypes( | ||
| // For scaleUpChron, we don't have a situation where the auth repo is different from the config repo | ||
| // so we can just pass the same repo for both parameters | ||
| repo, | ||
| repo, | ||
| scaleConfigRepo, |
There was a problem hiding this comment.
the comment up above this line should be updated
Currently we've been guessing org/repo to authenticate in most cases, this configuration when set both enforce validation and defaults to the given org/repo for authentication.
This enables scaleUpChron to run properly when the org/repo for the scale config is not the same as the one used to authenticate and being managed.
additional small fixes to remove warnings on linting that have been ignored.