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

aws-lambda: Log format option is not typed #28114

Closed
regoawt opened this issue Nov 22, 2023 · 2 comments · Fixed by #28942
Closed

aws-lambda: Log format option is not typed #28114

regoawt opened this issue Nov 22, 2023 · 2 comments · Fixed by #28942
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@regoawt
Copy link

regoawt commented Nov 22, 2023

Describe the bug

The logFormat option in the FunctionOptions interface is untyped, meaning that it does not accept the LogFormat enum.

See line 469 of https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda/lib/function.ts.

It has also been mentioned in #28039 (comment).

Expected Behavior

It should accept one of the LogFormat enum options.

Current Behavior

It doesn't accept the LogFormat enum so you have to specify a string "JSON" or "TEXT", which is not type-safe.

Reproduction Steps

N/A

Possible Solution

It should be typed as

readonly logFormat?: LogFormat;

Additional Information/Context

No response

CDK CLI Version

2.110.1

Framework Version

No response

Node.js Version

20.9.0

OS

MacOS

Language

Python

Language Version

3.11.2

Other information

No response

@regoawt regoawt added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 22, 2023
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Nov 22, 2023
@khushail
Copy link
Contributor

Hi @regoawt , thanks for reproting this.

@khushail khushail added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 24, 2023
@ConnorRobertson ConnorRobertson self-assigned this Jan 24, 2024
@mergify mergify bot closed this as completed in #28942 Jan 31, 2024
mergify bot pushed a commit that referenced this issue Jan 31, 2024
> # Issue
> > The issue is that LogFormat is a String so it doesn't allow the enum LogFormat. 
> # Solution
> > Created a new enum for the LoggingFormat and added testing. 
So the solution sets these values as potential environment variables.
The main difference is LoggingFormat is assigned to an enum instead of a string.
> # Important Design Decisions
> > This is so that an enum could be used for LoggingFormat without breaking JSII target languages. Some background information is in this pr #28127. Was a recommended solution here. #28127
>
> Remember to follow the [CONTRIBUTING GUIDE] and [DESIGN GUIDELINES] for any
> code you submit.
>
> [CONTRIBUTING GUIDE]: https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md
> [DESIGN GUIDELINES]: https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md

Closes #28114.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

mergify bot pushed a commit that referenced this issue Feb 2, 2024
### Issue # (if applicable)

Fixes bug in LoggingFormat `@default`

### Reason for this change
Incorrect
#28114 related issue


### Description of changes
Not much to describe here. 

### Description of how you validated changes

Shouldn't need any as there is integ tests from this pr
#28942

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)



BREAKING CHANGE: changing the default type of `lambda.loggingFormat`. Previously it was a string `"Text format"`, not it is an enum `LoggingFormat.TEXT`.



*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
SankyRed pushed a commit that referenced this issue Feb 8, 2024
> # Issue
> > The issue is that LogFormat is a String so it doesn't allow the enum LogFormat. 
> # Solution
> > Created a new enum for the LoggingFormat and added testing. 
So the solution sets these values as potential environment variables.
The main difference is LoggingFormat is assigned to an enum instead of a string.
> # Important Design Decisions
> > This is so that an enum could be used for LoggingFormat without breaking JSII target languages. Some background information is in this pr #28127. Was a recommended solution here. #28127
>
> Remember to follow the [CONTRIBUTING GUIDE] and [DESIGN GUIDELINES] for any
> code you submit.
>
> [CONTRIBUTING GUIDE]: https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md
> [DESIGN GUIDELINES]: https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md

Closes #28114.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
SankyRed pushed a commit that referenced this issue Feb 8, 2024
### Issue # (if applicable)

Fixes bug in LoggingFormat `@default`

### Reason for this change
Incorrect
#28114 related issue


### Description of changes
Not much to describe here. 

### Description of how you validated changes

Shouldn't need any as there is integ tests from this pr
#28942

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)



BREAKING CHANGE: changing the default type of `lambda.loggingFormat`. Previously it was a string `"Text format"`, not it is an enum `LoggingFormat.TEXT`.



*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TheRealAmazonKendra pushed a commit that referenced this issue Feb 9, 2024
> # Issue
> > The issue is that LogFormat is a String so it doesn't allow the enum LogFormat. 
> # Solution
> > Created a new enum for the LoggingFormat and added testing. 
So the solution sets these values as potential environment variables.
The main difference is LoggingFormat is assigned to an enum instead of a string.
> # Important Design Decisions
> > This is so that an enum could be used for LoggingFormat without breaking JSII target languages. Some background information is in this pr #28127. Was a recommended solution here. #28127
>
> Remember to follow the [CONTRIBUTING GUIDE] and [DESIGN GUIDELINES] for any
> code you submit.
>
> [CONTRIBUTING GUIDE]: https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md
> [DESIGN GUIDELINES]: https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md

Closes #28114.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TheRealAmazonKendra pushed a commit that referenced this issue Feb 9, 2024
### Issue # (if applicable)

Fixes bug in LoggingFormat `@default`

### Reason for this change
Incorrect
#28114 related issue


### Description of changes
Not much to describe here. 

### Description of how you validated changes

Shouldn't need any as there is integ tests from this pr
#28942

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)



BREAKING CHANGE: changing the default type of `lambda.loggingFormat`. Previously it was a string `"Text format"`, not it is an enum `LoggingFormat.TEXT`.



*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment