feat: support for setting environment secrets, and other task-def params#204
feat: support for setting environment secrets, and other task-def params#204pierre-wehbe wants to merge 2 commits intoaws-actions:masterfrom
Conversation
|
@clareliguori @hyandell @kdaily @awood45 @mattsb42-aws Can anyone take a look at this or know who can? |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
| containerDef.logConfiguration.options["awslogs-group"] = awslogsGroup; | ||
| containerDef.logConfiguration.options["awslogs-region"] = awslogsRegion; |
There was a problem hiding this comment.
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.
| 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: |
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
by "environment-secrets" you mean both value from SSM and from Secret Manager right ?
|
Is there any progress in getting this released? |
|
Any updates here? Having secrets, executionRoleArn, and taskRoleArn would be really nice to not have to maintain a separate |
|
Can this be merged any time soon? |
|
@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. |
|
Thank you! But I still think providing SSM name as variable is much smoother experience |
|
Is this something that can merged in now? I haven't looked to see the conflicts. |
|
Hi @pierre-wehbe, thank you so much for your contribution. Apologies on the delay.
|
|
Any update on this? |
10 participants
Description of changes: These commits add a few extra inputs (all optional)
I've updated the tests to include testing for all these.