Support "local down" to stop a running local task #783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manual tests1. Clean environmentThe user never ran $ ecs-cli local down
WARN[0000] Compose file docker-compose.local.yml does not exist in current directory
WARN[0000] Network ecs-local-network not found, skipping network removal
$2. No running local tasks, but network and endpoints container is up$ ecs-cli local down
WARN[0000] Compose file docker-compose.local.yml does not exist in current directory
INFO[0000] The network ecs-local-network has no more running tasks
INFO[0000] Stopped container with name amazon-ecs-local-container-endpoints
INFO[0000] Removed container with name amazon-ecs-local-container-endpoints
INFO[0000] Removed network with name ecs-local-network
$3. Stop a running task3.1. Stopping using
|
PettitWesley
reviewed
Jun 3, 2019
| @@ -46,6 +49,16 @@ func GetCommand(args []string) *exec.Cmd { | |||
| return exec.Command(cmdPath, args...) | |||
| } | |||
|
|
|||
| // RunCmd runs a command with args and returns its Stdout. | |||
| func RunCmd(t *testing.T, args []string) stdout.Stdout { | |||
PettitWesley
reviewed
Jun 3, 2019
| } | ||
|
|
||
| for _, container := range containers { | ||
| if err = docker.ContainerStop(context.Background(), container.ID, nil); err != nil { |
There was a problem hiding this comment.
Can we use a context with a timeout
There was a problem hiding this comment.
Yes! 🙈 done, moved docker.go to its own sub-package with an exported timeout constant.
PettitWesley
reviewed
Jun 3, 2019
| // Has other containers running in the network | ||
| logrus.Infof("%d other task(s) running locally, skipping network removal.", len(resp.Containers)-1) | ||
| // Has other containers running in the network, skip teardown. | ||
| logrus.Infof("%d other task(s) running locally, skipping network removal", len(resp.Containers)-1) |
There was a problem hiding this comment.
How do we know that the number of running tasks is equal to len(resp.Containers)-1
There was a problem hiding this comment.
The LocalEndpoints container should always be running in the network, so I subtract it to show the actual number of running containers for ECS local.
Updated the comment to:
// Don't count the endpoints container part of the running containers
There was a problem hiding this comment.
Got it. But tasks != containers. So I think it should say "other container(s) running locally"
There was a problem hiding this comment.
Yeah, I updated that too :)
PettitWesley
reviewed
Jun 3, 2019
| // Down stops and removes a running local ECS task. | ||
| // If the user stops the last running task in the local network then also remove the network. | ||
| func Down(c *cli.Context) error { | ||
| defer func() { |
PettitWesley
approved these changes
Jun 4, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
ecs-cli local downstops a running local task. If the stopped task is the last running ECS local task, then also teardown the network.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Documentation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.