Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Introduce ECS emulation mode #544

Merged
merged 5 commits into from
Sep 1, 2020
Merged

Introduce ECS emulation mode #544

merged 5 commits into from
Sep 1, 2020

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Aug 25, 2020

What I did
Introduce support for ECS simulation mode in the compose-cli with up --simulation command

Related issue
https://github.com/docker/ecs-plugin/issues/210

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

@ndeloof ndeloof changed the title Introduce ECS emulation mode [WiP] Introduce ECS emulation mode Aug 25, 2020
@ndeloof ndeloof changed the title [WiP] Introduce ECS emulation mode Introduce ECS emulation mode Aug 26, 2020
@ndeloof ndeloof requested a review from rumpl August 26, 2020 07:51
cli/cmd/compose/up.go Outdated Show resolved Hide resolved
cli/cmd/compose/up.go Outdated Show resolved Hide resolved
@ndeloof ndeloof changed the title Introduce ECS emulation mode [on hold] Introduce ECS emulation mode Aug 26, 2020
@ndeloof
Copy link
Collaborator Author

ndeloof commented Aug 26, 2020

I'm stopping work on this waiting for discussion on UX to be applied here

  • use of --simulation will require same flag on all other docker compose and might be confusing/anoying for user
  • introduction of a dedicated ecs-simulation context type could be an alternative

@ndeloof ndeloof force-pushed the simulation branch 2 times, most recently from 3e40d4e to c7b1591 Compare August 31, 2020 12:43
@ndeloof ndeloof changed the title [on hold] Introduce ECS emulation mode Introduce ECS emulation mode Aug 31, 2020
@@ -27,6 +27,8 @@ import (
"syscall"
"time"

"github.com/docker/compose-cli/cli/cmd/compose"
Copy link
Contributor

Choose a reason for hiding this comment

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

lol our imports are all borked

Signed-off-by: Nicolas De Loof <[email protected]>
@rumpl
Copy link
Contributor

rumpl commented Aug 31, 2020

Is this ready to be reviewed or do we wait for tests?

@ndeloof
Copy link
Collaborator Author

ndeloof commented Aug 31, 2020

I consider this to be ready for review. Can add tests if requested but not sure what should be covered

return cmd.Run()
}

func (e ecsLocalSimulation) Convert(ctx context.Context, project *types.Project) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a unit test on the global Convert method ?
Regarding e2e tests, I would feel more confident if we could run a compose up/down in the CI in simulation mode, but this will require docker-compose available on top of the plain docker (and the right version) ; we need to check how to set this up, does not need to be a blocker for this PR

Copy link
Contributor

@gtardif gtardif left a comment

Choose a reason for hiding this comment

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

LGTM
Couple of comments, mainly check how we can improve tests. (unit test might be too much copy/paste all static struct that needs to be added to compose file, an e2e test would much better validate that everything works nicely)

@gtardif
Copy link
Contributor

gtardif commented Sep 1, 2020

better understanding after reading https://aws.amazon.com/blogs/compute/a-guide-to-locally-testing-containers-with-amazon-ecs-local-endpoints-and-docker-compose/.
Effectively a bit heavy to test (for not much functionality, appart getting the creds from the local aws account)

@gtardif gtardif merged commit c7e2db0 into main Sep 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants