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: support for setting environment secrets, and other task-def params #204

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

Conversation

pierre-wehbe
Copy link

Description of changes: These commits add a few extra inputs (all optional)

  • family
  • cpu
  • memory
  • executionRoleArn
  • taskRoleArn
  • awslogsGroup
  • awslogsRegion
  • environmentSecrets

I've updated the tests to include testing for all these.

@joshuabalduff
Copy link

joshuabalduff commented Nov 17, 2022

@clareliguori @hyandell @kdaily @awood45 @mattsb42-aws Can anyone take a look at this or know who can?

Comment on lines +23 to +42
description: 'task-def family'
required: false
cpu:
description: 'CPU'
required: false
memory:
description: 'Memory'
required: false
executionRoleArn:
description: 'executionRoleArn'
required: false
taskRoleArn:
description: 'taskRoleArn'
required: false
awslogs-group:
description: 'awslogs-group'
required: false
awslogs-region:
description: 'awslogs-region'
required: false

Choose a reason for hiding this comment

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

Question; is the expectation here that some of this changes between github action execution?

Until now, we haven't added these fields into the github action config because we supply a task def json and then we read the parameters that change and inline them into the task definition. But I'm wondering if there is a usecase that we've missed.

Choose a reason for hiding this comment

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

Yeah some of these might not be needed like the logs, and task arns since you can just use the names. To be honest I think the biggest item needed is secret arns since they do contain the AWS Account Id and would prefer not to commit that in the repo.

}

// Get pairs by splitting on newlines
environmentSecrets.split('\n').forEach(function (line) {

Choose a reason for hiding this comment

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

Nit; this could be turned into a .map() that maps each environment secret line to type { name: string, valueFrom: string } and then we could merge these values into containerDef.secrets outside of the forEach loop.

})
}

if (awslogsGroup && awslogsRegion && containerDef.logConfiguration && containerDef.logConfiguration.options) {

Choose a reason for hiding this comment

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

These options look specific to awslogs log driver. I believe we should check if the logConfiguration.logsDriver is set to awslogs before we change these properties in the task def.

Comment on lines +141 to +142
containerDef.logConfiguration.options["awslogs-group"] = awslogsGroup;
containerDef.logConfiguration.options["awslogs-region"] = awslogsRegion;

Choose a reason for hiding this comment

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

Nit; It might also be worthwhile to also include the option to set awslogs-stream-prefix and awslogs-create-group as well because that'll ship the complete set of AWS logs options for customers.

@@ -16,6 +16,30 @@ inputs:
environment-variables:
description: 'Variables to add to the container. Each variable is of the form KEY=value, you can specify multiple variables with multi-line YAML strings.'
required: false
environment-secrets:

Choose a reason for hiding this comment

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

@joshmello can you think about how to add option to get secrets from AWS AppConfig? For example. we can set arn of config. It's will be very useful. Or just read env from file.
(asking because have a lot of env to setup)

Copy link

Choose a reason for hiding this comment

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

by "environment-secrets" you mean both value from SSM and from Secret Manager right ?

@jameshopkins
Copy link

Is there any progress in getting this released?

@OzzieOrca
Copy link

Any updates here? Having secrets, executionRoleArn, and taskRoleArn would be really nice to not have to maintain a separate .json file for each environment.

@sh-mykola
Copy link

Can this be merged any time soon?

@OzzieOrca
Copy link

@sh-mykola see #252 (comment) for another option using https://github.com/marketplace/actions/update-json-file to manually edit a JSON file (outside any AWS context), since that's all that really needs to be done.

@sh-mykola
Copy link

Thank you! But I still think providing SSM name as variable is much smoother experience

@joshuabalduff
Copy link

Is this something that can merged in now? I haven't looked to see the conflicts.

@amazreech
Copy link
Contributor

Hi @pierre-wehbe, thank you so much for your contribution. Apologies on the delay.
We will be working on reviewing Pull Requests on the repository. 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 if any.

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