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

feat: Add support for task and execution role ARNs in action.yml and … #293

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

Conversation

DMEvanCT
Copy link

@DMEvanCT DMEvanCT commented Apr 24, 2024

…index.js

Issue #, if available:

Description of changes:
Adding the ability to do set task role arn and the execution role arn. This may be different from account to account so setting it would provide a little flexibility.

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

@DMEvanCT DMEvanCT closed this Apr 24, 2024
@DMEvanCT DMEvanCT reopened this Apr 24, 2024
@amazreech
Copy link
Contributor

amazreech commented May 10, 2024

Hi @DMEvanCT, thank you so much for your contribution. Apologies on the delay. We will be working on reviewing Pull Requests on the repositories. In the mean time please ensure that below steps, if not already done, are taken care of in your PR:

  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 merge conflicts on the PR

@@ -38,6 +41,7 @@ async function run() {
}
containerDef.image = imageURI;


Choose a reason for hiding this comment

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

nit: could you remove unnecessary whitespace?

@@ -16,6 +16,9 @@ async function run() {
const logConfigurationOptions = core.getInput("log-configuration-options", { required: false });
const dockerLabels = core.getInput('docker-labels', { required: false });
const command = core.getInput('command', { required: false });
const taskRoleArn = core.getInput('task-role-arn', { required: false });
const executionRoleArn = core.getInput('execution-role-arn', { required: false });

Choose a reason for hiding this comment

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

nit: could you remove unnecessary whitespace?

description: 'The ARN of the IAM role that the ECS container will assume'
required: false
execution-role-arn:
description: 'The ARN of the IAM role that the ECS task execution role will assume'

Choose a reason for hiding this comment

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

This description can be made more explicit, as it could confuse some users since it's self-referencing.
Suggested reword: 'The ARN of the IAM role that the task definition will assume to run the task'

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.

3 participants