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

Refactor templates to have fewer imported values #37

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

cat5inthecradle
Copy link
Contributor

@cat5inthecradle cat5inthecradle commented Jan 12, 2024

!ImportValue adds cross-stack dependencies and breaks the cleaner top-down passing of parameters. This PR eliminates !ImportValue from the application stack via two changes:

  • The CICD pipeline stack tells the app stacks what VPC, security group, and subnets to use
  • The app stack creates it's own ECS Task Execution Role

This also modifies the deploy scripts to create a change set which can be reviewed before applying.

Notes for reviewers - the shell script changes are independent of the template changes, but having them in place will make reviewing the template changes easier.

TESTING

  • TODO deploy a both a dev environment and a prod-like pipeline to validate that the parameter overrides are formatting correctly.

Follow Up

  • delete the now redundant ecs task execution role being created in the dependencies template AFTER all stacks are updated to use their own role.

@@ -27,7 +27,6 @@ Resources:
- codepipeline.amazonaws.com
Version: '2012-10-17'
Path: /service-role/
PermissionsBoundary: !ImportValue IAM-DevPermissions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tentative removal. I don't think we need it here. This stack is being deployed by an Admin anyway. I might be overlooking something though.

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.

1 participant