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

Support for using existing or Terraform-managed task definitions #518

Conversation

filipemacedo
Copy link

@filipemacedo filipemacedo commented Sep 12, 2023

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:

  • Added the capability to specify an existing task definition in Terraform to be deployed, avoiding the creation of new definitions.
  • Updated documentation to guide users on how to use this new functionality.
  • Added additional tests to ensure the integrity and reliability of the action

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@filipemacedo
Copy link
Author

@hyandell @clareliguori hey, is this repository still accepting contributions?

@bmax
Copy link

bmax commented Feb 16, 2024

This would be helpful!

@fabiobarboza7
Copy link

fabiobarboza7 commented Feb 28, 2024

That's so helpful! I wish it to be merged. Great job @filipemacedo

@filipemacedo
Copy link
Author

Hey @KollaAdithya, do I need to do anything?

@amazreech
Copy link
Contributor

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:

  1. Verify if PR follows semantic pull request conventions.
    Example: please make sure the PR title follows the convention.

  2. Please run npm run package command to update dist/ folder with latest dependencies.

  3. Resolve any merge conflicts on the PR

try {
const describeResponse = await ecs.describeTaskDefinition({
taskDefinition: taskDefinitionFile
}).promise();
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

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

@fabien-renaud-velco
Copy link

I'm glad that you have implemented this feature @filipemacedo, thank you! I wish it can be merged

Copy link
Contributor

@trivikr trivikr left a 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.

Comment on lines +290 to +292
const describeResponse = await ecs.describeTaskDefinition({
taskDefinition: taskDefinitionFile
}).promise();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
registerResponse = await ecs.registerTaskDefinition(taskDefContents).promise();
registerResponse = await ecs.registerTaskDefinition(taskDefContents);

@filipemacedo
Copy link
Author

Provided diff for using v3 in source code. Equivalent changes need to be done in test file while rebasing.

Thank you. I will implement the requested changes this week.

@shesaave
Copy link
Contributor

Hi @filipemacedo,
Thank you for your Patience. We appreciate your contribution. We noticed the contributions were made in deploy repository but we have a separation of logic between render and deploy repositories such that, render fetches/updates the task definition and deploy deploys it to ECS. We believe, the logic for fetching task definition should live in the render repository. It is also one of our heavily requested features in render #102 . We will be working on this feature in the render repository. I will be closing this PR now, but please feel free to open PR or issues with other features or concerns.

@shesaave shesaave closed this Jun 17, 2024
@fabiobarboza7
Copy link

@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.

@karanbokil
Copy link

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.

@fabien-renaud-velco
Copy link

fabien-renaud-velco commented Jun 26, 2024

Hi @shesaave and @karanbokil,
This pull request intends to allow developers to deploy an existing task definition, instead of register a new task definition, then deploy it. In both scenarios, the objective is to deploy a task definition to an ECS service. We already have the feature to register task definition without deploying it, these features go hand to hand. I don't think this feature breaks the separation of concerns that you want to maintain in render / deploy action repositories. In addition, I can't imagine a way to implement this in render repository.

@karanbokil
Copy link

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 amazon-ecs-render-task-definition action, which is one of the most requested features by the community #102. These actions are meant to work together and I feel the best course of action is to allow amazon-ecs-render-task-definition to be the responsible actor for interacting with fetching task-definitions since it further enables additional workflows like fetching an existing task definition then optionally adjusting that task-definition with a new image. By placing the feature in the amazon-ecs-render-task-definition action, we are able to help more developers achieve their desired workflows. Do let me know if you have other questions or would like to help participate in the feature integration in the render-task-definition repo.

@fabien-renaud-velco
Copy link

@karanbokil I totally agree that fetching task-definition would not make any sense, and it is a feature more suitable to the amazon-ecs-render-task-definition action (I just upvoted it, thank you). But this feature is not about fetching task-revision, only deploying task-definition by its <family>:<revision> identifier. It would allows the following scenario:

Workflow A - Create task-definition

  • Push Docker image using docker/build-push-action
  • Fetch and render task-definition using aws-actions/amazon-ecs-render-task-definition
  • Register task-definition using aws-actions/amazon-ecs-deploy-task-definition without specifying service

Workflow B - Deploy task-definition

  • Deploy task-definition using aws-actions/amazon-ecs-deploy-task-definition by specifying <family>:<revision> as task-definition parameter instead of a file, or just <family> if you want to deploy latest revision

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 amazon-ecs-render-task-definition would not satisfy this requirement.

@karanbokil
Copy link

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 source_code -> image -> ecr -> task-definition -> ecs deployment journey. Our prior reasoning to close the PR was that render-task-definition could just as easily fetch an existing task definition to help developers still inject their image with the same flow above without needing to check in a task-definition.json.

My confusion stems from the fact that this is assuming terraform -> task-definition to replace the above flow almost entirely and simply using this action now to deploy task-definitions to ECS.

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?

@fabien-renaud-velco
Copy link

fabien-renaud-velco commented Jun 27, 2024

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 @aws-actions/amazon-ecs-deploy-task-definition. Doing so would make it possible to deploy both the latest pending version or whichever existing version for rollback without having to register a new one.

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.

9 participants