This repository has been archived by the owner on Jan 31, 2024. It is now read-only.
[back-357] add ECS service, IAM, & definition modules #36
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
jpetto
force-pushed
the
back-357-complete-ecs-service-module
branch
2 times, most recently
from
01e08c5
to
90ee018
Compare
jpetto
force-pushed
the
back-357-complete-ecs-service-module
branch
from
90ee018
to
e0314b0
Compare
bassrock
reviewed
Oct 30, 2020
| const secretEnvVarsValue = config.secretEnvVars | ||
| ? JSON.stringify(config.secretEnvVars) | ||
| : 'null'; | ||
| const commandValue = config.command |
There was a problem hiding this comment.
Do the amazon docs say what to put if we don't want to override this? or does an existing task definition provide an example? Just want to make sure an empty string means don't override.
There was a problem hiding this comment.
checked and it's an optional param. updating to remove it from the JSON if no command is configured.
|
|
||
| new ApplicationECSIAM(stack, 'testECSService', BASE_CONFIG); | ||
|
|
||
| expect(Testing.synth(stack)).toMatchSnapshot(); |
src/base/ApplicationECSService.ts
Outdated
|
|
||
| //create ecs service | ||
| new EcsService(this, 'ecs-service', { |
There was a problem hiding this comment.
we will need to reference this elsewhere, can you make it public on the class
src/pocket/PocketALBApplication.ts
Outdated
| new ApplicationECSService(this, 'ecs_service', { | ||
| prefix: config.prefix, | ||
| name: config.alb6CharacterPrefix, | ||
| ecsCluster: '', |
There was a problem hiding this comment.
You should actually be able to use ApplicationECSCluster and create it before this :D
- make ecs service public - remove command from container def if none passed in - update snapshots
…ket/terraform-modules into back-357-complete-ecs-service-module
kkyeboah
approved these changes
Nov 3, 2020
|
🎉 This PR is included in version 1.3.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 tasks
2 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Goal
add ECS service, IAM, and container definition modules.
this work has been validated through the following:
npm run build && npm run synthcd cdktf.outterraform initterraform validatei haven't yet tested all this work against our dev infra (but i have been testing along the way and fixing some issues).
imo, the "worst" part about using this ECS module will be passing in and testing the role policy statements for ECS IAM. (these will be a part of the consuming project).
one other thing that's getting a bit hairy is the size of the config objects, as we need to pass in and down quite a bit of info from the entry point into sub-modules (e.g. ECSService > ECSIAM and ECSService > ECSContainerDefinition). an alternative would be to have these all top-level modules that are configured separately and combined within ECSService. might be a point to discuss.
Todos
terraform planandterraform applyagainst our dev infraReference
Tickets:
Implementation Decisions
attempted to abstract/make configurable as much as practical at this time. i'm sure there will be changes to what is configurable going forward.