-
Notifications
You must be signed in to change notification settings - Fork 330
Conversation
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 can't believe this is the right way to do this but I've seen this done in Terraform and so many other places that it just must be. LOL.
@@ -1063,58 +1056,29 @@ func (p *Platform) Launch( | |||
L.Debug("registering task definition", "id", id) | |||
|
|||
var cpuShares int | |||
family := "waypoint-" + app.App | |||
|
|||
s.Status("Registering Task definition: %s", family) |
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.
Note: I moved this because it's the first status update in this method, and without moving it (or adding another one) when the validation fails it looks like the previous step failed, and not this one
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.
Looks good, just a question about the changelog file.
```release-note:bug | ||
plugin/aws/ecs: validate memory and cpu values | ||
``` |
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 think these should be in one block, mostly because I think go-changelog only supports one block per file?
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.
You can have multiple entries in a single go-changelog txt file! https://github.com/hashicorp/waypoint/blob/main/.github/CHANGELOG_GUIDE.md#multiple-entries
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.
AWS ECS / Fargate require the memory and cpu values for containers to be agreeable, and will error when trying to create a container or task definition if they do not match the prescribed values. See this article for more information:
The Waypoint ECS deployment already validated these to a point, but the ECS Server install did not. This PR refactors that validation into a shared utility and uses it for both the platform and server install.
UX for using bad memory value in an ECS deploy:
UX for using bad memory / cpu values for server install: