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

Option to Directly Specify taskDefinitionArn as an Input Parameter #524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

miltonhit
Copy link

@miltonhit miltonhit commented Oct 21, 2023

Description of changes:
Added conditional logic to support both direct task definition ARN or task definition files in ECS task registration.

With this enhancement, you will be able to create or register your task definition using external tools like CloudFormation, Amazon CDK, or the Serverless Framework. Once registered, simply submit it here for streamlined deployment and await a successful response.

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

@miltonhit miltonhit changed the title possibility Option to Directly Specify taskDefinitionArn as an Input Parameter Oct 21, 2023
@fabn
Copy link

fabn commented Jan 3, 2024

I was looking for this option, is it something that has been considered for merge?

@crissdev
Copy link

It would be great to have that feature.

@miltonhit
Copy link
Author

@fabn @crissdev
Unfortunately this repository is abandoned :(

I've done this change in a fork.
If you want to use or fork:

    - name: Deploy to Amazon ECS
      uses: miltonhit/[email protected]
      with:
        task-definition: arn:aws:ecs:<region>:<aws_account_id>:task-definition/<task_definition_name>:<revision_number>
        service: my-service
        cluster: my-cluster
        wait-for-service-stability: true

Repo: https://github.com/miltonhit/amazon-ecs-deploy-task-definition

@crissdev
Copy link

thanks @miltonhit. I'm currently using some of that in my fork.

Copy link

@junioroo junioroo left a comment

Choose a reason for hiding this comment

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

LGTM

@amazreech
Copy link
Contributor

Hi @miltonhit,

Thank you for your Patience. Apologies for the delay. We appreciate your contribution to the repository and will be working to review the changes in the Pull Request. 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.

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

  3. Resolve any merge conflicts on the PR

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.

This PR needs to be updated to use AWS SDK for JavaScript (v3)

Refer: #529

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);

@LukeSheard
Copy link

LukeSheard commented Oct 8, 2024

Is there a way we can get this merged? This is support I'd love to see. If @miltonhit isn't responsive here should I open a new PR which resolves the conflicts?

Edit: I raised a PR here which implements this in a way which is compatible with the v3 API. We've started using this internally so I'm pretty confident it works sufficiently.

@s3cube
Copy link
Contributor

s3cube commented Oct 11, 2024

Thanks for your PR @LukeSheard! Appreciate the contribution. Taking a look

@LukeSheard
Copy link

Hey @s3cube any update on being able to get my PR merged in? 🤞

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