-
Notifications
You must be signed in to change notification settings - Fork 27
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
Scaler cleanups #188
Scaler cleanups #188
Conversation
Rearrange the loop so that scaler.Run happens first. This also lets us check for timeout while sleeping.
log.Printf includes newline Also we're now waiting for timeout or sleep
* Build container (Makefile) * Pipeline containers (pipeline.yaml) * go.mod * go get -u && go mod tidy
e897efd
to
362507e
Compare
4765921
to
5a27c3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor observations and nitpicks.
lambda/main.go
Outdated
agentsPerInstanceStr := os.Getenv("AGENTS_PER_INSTANCE") | ||
if agentsPerInstanceStr == "" { | ||
return "", errors.New("AGENTS_PER_INSTANCE is required") | ||
} | ||
agentsPerInstance, err := strconv.Atoi(agentsPerInstanceStr) | ||
if err != nil { | ||
return "", fmt.Errorf("AGENTS_PER_INSTANCE must be an integer: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a function for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored the environment variable loading back towards a few helper functions, which I feel also resolves the comments below. Since they all error out of the lambda handler, making it useless, I've leaned towards log.Fatalf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
This is a grab-bag of code cleanups.
The only significant change should be that
scaler.Run
now happens before checkingtimeout
, so the lambda won't time out before it does anything useful (regardless ofLAMBDA_TIMEOUT
).