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

Use the stack-name in the Name property of the ContainerDefinition #226

Merged
merged 3 commits into from
Jun 21, 2018

Conversation

arunasank
Copy link
Contributor

Fixes #213

Currently, the options.service variable is required to be a string since it's concatenated with the options.prefix to create the Name property of the container. This PR removes that requirement and allows the options.service to be a Cloudformation reference. Besides the flexibility that this allowance gives us, it is also useful for differentiating containers of different stacks of the same repository, since the options.service can be referenced to AWS::StackName now, for example.

@arunasank arunasank changed the title Allow for options.service to be a CFN reference [WIP] Allow for options.service to be a CFN reference Jun 20, 2018
@arunasank
Copy link
Contributor Author

Decided to remove the options.service dependency altogether from the Name property of the container, since the options.service property if also used to derive the name of the ECR repository for a stack. Using the cf.stackName reference directly in the Name property, to avoid clashing with the options.service.

@arunasank
Copy link
Contributor Author

Additional tests are not required, since the changes use a cloudformation reference in the container name (and don't modify the datatype of the options.service variable as intended earlier), and the fact that the existing tests in template.validation.test.js pass is enough proof that the changes don't introduce validation errors in the cloudformation template. I've also tested this on a watchbot stack and double-confirmed that there are no validation errors, and that the name of the container is as expected.

@arunasank arunasank changed the title [WIP] Allow for options.service to be a CFN reference Use the stack-name in the container name Jun 20, 2018
@arunasank arunasank changed the title Use the stack-name in the container name Use the stack-name in the Name property of the ContainerDefinition Jun 20, 2018
@arunasank
Copy link
Contributor Author

cc/ @mapbox/platform-engine-room

Copy link

@jakepruitt jakepruitt left a comment

Choose a reason for hiding this comment

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

Looks great!

@arunasank arunasank merged commit 783a9e4 into master Jun 21, 2018
@arunasank arunasank deleted the containerName branch June 21, 2018 07:07
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.

Use the stack name in the containerName
2 participants