fix(ecs): configFileValue on FirelensOptions should be an optional property #16856
Conversation
|
|
david-doyle-as24
changed the title
peterwoodworth
changed the title
github-actions
bot
added
the
@aws-cdk/aws-ecs
There was a problem hiding this comment.
Hi @david-doyle-as24 , awesome PR! Thanks for working on this.
I just have two small suggestions!
| @@ -78,7 +79,7 @@ export interface FirelensConfig { | |||
|
|
|||
| /** | |||
| * Firelens options | |||
| * @default - no additional options | |||
| * @default - EnableECSLogMetadata option is set to true | |||
There was a problem hiding this comment.
nit: @default - ECS log metadata enabled
| } else if (options?.configFileType && !options?.configFileValue) { | ||
| throw new Error('Firelens Config Error: Config file value cannot be undefined when config file type is declared.'); |
There was a problem hiding this comment.
Can we also add a unit test to assert this error being thrown?
| @@ -116,7 +117,7 @@ function renderFirelensConfig(firelensConfig: FirelensConfig): CfnTaskDefinition | |||
| options: { | |||
There was a problem hiding this comment.
I think we can also remove the if-else condition here to always return the firelens config in the else part (as the enable-ecs-log-metadata in the options will always be present now).
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
It seems from the Build Logs like the |
madeline-k
changed the title
rix0rrr
added
bug
|
Hello, I am closing this PR since it is in a build-failing state, and has not been touched in a long time. If anyone is still interested to contribute a fix here, please open a new PR. |
Firelens config mistakenly requires configFileType, when this documentation allows that any of firelens config options can be set or unset. This PR fixes this bug and sets the "enable-ecs-log-metadata" field by default to true, even when the options field is not set.
Closes #16594
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license