Add utilities for ecs. #73
Conversation
awscommons/v2/ecs.go
Outdated
| logger := logging.GetProjectLogger() | ||
| logger.Infof("Looking up Container Instance ARNs for ECS cluster %s", clusterName) |
There was a problem hiding this comment.
Like the autoscaling PR, the logger should be injected in the Options struct.
awscommons/v2/ecs.go
Outdated
| input := &ecs.ListContainerInstancesInput{Cluster: aws.String(clusterName)} | ||
| arns := []string{} | ||
| // Handle pagination by repeatedly making the API call while there is a next token set. | ||
| for { |
There was a problem hiding this comment.
NIT: consider adding a maximum to the loop iteration to avoid an infinite loop. Logically, the AWS API should prevent such an infinite loop, but it's always good to practice defensive coding to avoid unknown unknowns causing unexpected behaviors.
There was a problem hiding this comment.
Ah, that's true. I'll take care of this in the next review cycle, putting a todo here for now.
awscommons/v2/ecs.go
Outdated
| } | ||
|
|
||
| // Yay, all done. | ||
| if drained, _ := allInstancesFullyDrained(responses); drained == true { |
There was a problem hiding this comment.
This should handle the error or add comment as to why it's ok not to handle the error.
awscommons/v2/ecs.go
Outdated
| // If there's no error, retry. | ||
| if err == nil { | ||
| return errors.WithStackTrace(fmt.Errorf("container instances still draining")) | ||
| } | ||
|
|
||
| // Else, there's an error, halt and fail. | ||
| return retry.FatalError{Underlying: err} |
There was a problem hiding this comment.
This err check is a tautology, since err is only set within the for loop above, and you immediately return err if there is an error. So it will always be nil at this point.
You can reduce this to return the container instances still draining error.
There was a problem hiding this comment.
Ah, I see. I think the best thing is to retry in all other cases. If timeout exceeded, exit with error. If drained, return nil. All other cases, retry.
rhoboat
force-pushed
the
feature/ecs
branch
from
af3384e
to
a8078c0
Compare
|
Ah, Build failure, fixing... |
|
Let's see if that adequately fixed the merge conflict... |
Description
Add utilities for ecs.
This isn't quite ready--we probably need tests.
Documentation
TODOs
Related Issues
Related to https://github.com/gruntwork-io/ecsgrunt/pull/2.