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: ability to configure log driver and log options #255

Merged
merged 4 commits into from
Sep 6, 2023

Conversation

sombriks
Copy link
Contributor

Issue #254

Description of changes:

For my workflow i need to override the awslogs-group value so it doesn't clash with other group names. app on dev environment and app on staging would override messages if the task definition file is shared between them, and turns out it is.

By having a chance to override log configuration i can rename the property without having to keep different versions of the same task definiton.

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

Copy link
Contributor

@KollaAdithya KollaAdithya left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ❤️ ! I just left some comments feel free to address those comments before we can merge the PR.

index.js Outdated
@@ -70,6 +73,29 @@ async function run() {
})
}

if (logConfigurationLogDriver) {
if (!containerDef.logConfiguration) { containerDef.logConfiguration = {} }
const validDrivers = ["json-file", "syslog", "journald", "gelf", "fluentd", "awslogs", "splunk", "awsfirelens"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently for the ECS task, these are the log configurations that are supported.

For tasks on AWS Fargate, the supported log drivers are awslogs, splunk, and awsfirelens
For tasks hosted on Amazon EC2 instances, the supported log drivers are awslogs, fluentd, gelf, json-file, journald, logentries, syslog, splunk, and awsfirelens.

Are we assuming these drivers for ECS Tasks based on Ec2 instance? if this is the case do we need to add logentries as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my current scenario is for fargate but yes, let's add logentries!

index.js Outdated
if (logConfigurationLogDriver) {
if (!containerDef.logConfiguration) { containerDef.logConfiguration = {} }
const validDrivers = ["json-file", "syslog", "journald", "gelf", "fluentd", "awslogs", "splunk", "awsfirelens"];
if (!validDrivers.find(driver => driver === logConfigurationLogDriver)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be simplified?

if (!validDrivers.includes(logConfigurationLogDriver)){
        throw new Error(`'${logConfigurationLogDriver}' is invalid logConfigurationLogDriver. valid options are ${validDrivers}. More details: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LogConfiguration.html`)
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i could remove the link to the documentation or the valid options list. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

what i mean by this comment is that

instead of using array function in L79. if (!validDrivers.find(driver => driver === logConfigurationLogDriver))
can you just use if (!validDrivers.includes(logConfigurationLogDriver))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh right away!

@KollaAdithya
Copy link
Contributor

Looks like i do not have any write access to merge the PR. I will contact the right person to merge this PR.

@iamhopaul123 iamhopaul123 merged commit c6f3dfb into aws-actions:master Sep 6, 2023
1 check passed
@tetienne-zenchef
Copy link

Hi, it would be nice to create a release that contains this feature. Thx.

@jonathanhuang13
Copy link

Yeah, can we create a release? Looks like a release hasn't been created since January

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.

None yet

5 participants