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

Add utilities for ecs. #73

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add utilities for ecs. #73

wants to merge 7 commits into from

Conversation

rhoboat
Copy link
Contributor

@rhoboat rhoboat commented Sep 24, 2022

Description

Add utilities for ecs.
This isn't quite ready--we probably need tests.

Documentation

TODOs

  • Update the docs.
  • Keep the changes backward compatible where possible.
  • Run the tests and checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.

Related Issues

Related to https://github.com/gruntwork-io/ecsgrunt/pull/2.

yorinasub17
yorinasub17 previously approved these changes Sep 26, 2022
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM! Had a few nits.

Comment on lines 32 to 33
logger := logging.GetProjectLogger()
logger.Infof("Looking up Container Instance ARNs for ECS cluster %s", clusterName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the autoscaling PR, the logger should be injected in the Options struct.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's true. I'll take care of this in the next review cycle, putting a todo here for now.

}

// Yay, all done.
if drained, _ := allInstancesFullyDrained(responses); drained == true {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should handle the error or add comment as to why it's ok not to handle the error.

Comment on lines 141 to 147
// 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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

yorinasub17
yorinasub17 previously approved these changes Sep 28, 2022
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

yorinasub17
yorinasub17 previously approved these changes Sep 28, 2022
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rhoboat
Copy link
Contributor Author

rhoboat commented Sep 28, 2022

Ah, Build failure, fixing...

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! But it looks like there is a conflict now since you merged in the asg updates.

@rhoboat
Copy link
Contributor Author

rhoboat commented Sep 28, 2022

Let's see if that adequately fixed the merge conflict...

@rhoboat rhoboat mentioned this pull request Sep 28, 2022
4 tasks
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.

2 participants