-
Notifications
You must be signed in to change notification settings - Fork 245
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
Support for using existing or Terraform-managed task definitions #518
Support for using existing or Terraform-managed task definitions #518
Conversation
…inition Fix/use the existing task definition
1a708c3
to
d359f41
Compare
@hyandell @clareliguori hey, is this repository still accepting contributions? |
This would be helpful! |
That's so helpful! I wish it to be merged. Great job @filipemacedo |
Hey @KollaAdithya, do I need to do anything? |
Hi @filipemacedo, Thank you for your Patience. We appreciate your contribution to the repository and will be working to review the changes in the Pull Request. We will reach-out if with any questions or suggestions during the review. In the meantime please ensure that below steps, if not already completed, are taken care of in your Pull Request:
|
try { | ||
const describeResponse = await ecs.describeTaskDefinition({ | ||
taskDefinition: taskDefinitionFile | ||
}).promise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs to be updated to use AWS SDK for JavaScript (v3)
Refer: #529
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trivikr ok
I'm glad that you have implemented this feature @filipemacedo, thank you! I wish it can be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided diff for using v3 in source code.
Equivalent changes need to be done in test file while rebasing.
const describeResponse = await ecs.describeTaskDefinition({ | ||
taskDefinition: taskDefinitionFile | ||
}).promise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const describeResponse = await ecs.describeTaskDefinition({ | |
taskDefinition: taskDefinitionFile | |
}).promise(); | |
const describeResponse = await ecs.describeTaskDefinition({ | |
taskDefinition: taskDefinitionFile | |
}); |
const taskDefContents = maintainValidObjects(removeIgnoredAttributes(cleanNullKeys(yaml.parse(fileContents)))); | ||
let registerResponse; | ||
try { | ||
registerResponse = await ecs.registerTaskDefinition(taskDefContents).promise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registerResponse = await ecs.registerTaskDefinition(taskDefContents).promise(); | |
registerResponse = await ecs.registerTaskDefinition(taskDefContents); |
Thank you. I will implement the requested changes this week. |
Hi @filipemacedo, |
@shesaave, seriously? All effort done and people using, required changes, refactores, and you just decide to close the pr because of that? I'm sure this is a bucket of cold water for us, developers ,regarding our motivation to contribute. |
Hi @filipemacedo and @fabiobarboza7, thank you for bringing forward your concern. I'm sorry for the rather abrupt closing of your PR after all the work you put into it, @shesaave and I were working on organizing issues across the ECS github actions and determined that fetching task definitions should belong in the amazon-ecs-render-task-definition repo as a proper separation of concerns. I understand and value all the efforts that were made in this PR and want to make sure such features are put in the correct home. If you are open to it, I would like to invite you to participate in the feature integration in the amazon-ecs-render-task-definition repo. Additionally, I am going to make sure that you both are named as contributors on the new PR, so that your work is properly credited, even if you choose not to participate. |
Hi @shesaave and @karanbokil, |
Hi @fabien-renaud-velco, thank you for your insight! While you are correct that in both scenarios the ultimate goal is to deploy a task definition, it would be redundant to include the ability to fetch an existing task definition in this Github Action while also having the ability to fetch an existing task definition in the |
@karanbokil I totally agree that fetching task-definition would not make any sense, and it is a feature more suitable to the Workflow A - Create task-definition
Workflow B - Deploy task-definition
The ultimate goal of this feature is to uncouple task-definition creation and task-definition deployment processes, so developers can have more flexibility in how they create task-definition (using GitHub actions or third-party softwares like Terraform) and when to deploy it. Adding the possibility to fetch task-definition in |
Hi @fabien-renaud-velco thanks for the clarifications, this is useful to my understanding of your use case with this. Traditionally, we have assumed the intention of the actions as a ci/cd tool to help inject a new image (via render-task-definition) and then register and deploy (via deploy-task-definition), which allows a seamless My confusion stems from the fact that this is assuming I would like to understand more about the why for this feature specifically. In what situations will deploying already existing task-definitions via github actions without making any modifications to it be helpful or not already convenient in terraform? |
Hi @karanbokil, I understand your confusion about Terraform, I probably shouldn't have mentioned it earlier. To clarify how I use these tools, I personally only use Terraform to provision the infrastructure resources (ECS cluster, ECS service(s), VPC subnets, IAM roles etc.) and render / deploy a default task-definition that is intended to be replaced. Service initialization is the only time I use Terraform to deploy a task-definition. Then I use GitHub actions as described above to render and deploy new task-definition revisions with new images and new environment configurations. Now to answer your question, I practice continuous delivery, but not continuous deployment for my production environment. I want to be able to build and test images in my CI, but in some cases (peak usage in production, dependant service not ready to deploy yet), you might want to delay your deployment. This is why I designed my deployment process in two distinguish workflows: one to build image, render and register task-definition (let's call it "build workflow"), second to deploy it when planets are aligned (deploy workflow). You could be tempted to render task-definition in the "deploy worfklow", but I personally injects some of my GitHub environment variables in my task-definition and register secrets to parameter store in my "build workflow". Rendering task-definition in the "deploy workflow" would implies that GitHub environment variables and secrets might change in the meantime, which is not expected. Also, I plan to include in my "deploy workflow" process a step to search for a task-definition revision based on image tag (which would be a semantic version, and identical to GitHub release tag), then deploy it using |
Issue #: #299
Description of changes:
Currently, the action creates new task definitions, which can result in desynchronization with configurations managed by tools like Terraform. With this update, users now have the ability to use existing task definitions managed by Terraform, providing greater cohesion and consistency in their deployment environments.
Key Changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.