-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(lambda): update the type of LogFormat option #28127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
@@ -466,7 +466,7 @@ export interface FunctionOptions extends EventInvokeConfigOptions { | |||
* Sets the logFormat for the function. | |||
* @default Text format | |||
*/ | |||
readonly logFormat?: string; | |||
readonly logFormat?: LogFormat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a breaking change since it's currently possible (arguably encouraged by the type) to set the property to the strings "JSON" or "Text".
I suggest adding | keyof LogFormat
which would let the user use either the enum or type-safe strings
readonly logFormat?: LogFormat; | |
readonly logFormat?: LogFormat | keyof typeof LogFormat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment.
I will modify logFormat type to accept both the enum and type-safe strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your suggestion.
Your comment realised me that my first change would be a breaking change.
Both of my first change and your suggestion can't accept both of the enum or type-safe strings.
Therefore, I changed the type of logFormat
as readonly logFormat?: LogFormat | `${LogFormat}`;
due to accept both of lambda.LogFormat.TEXT
and string 'Text'
.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we add validation that the text passed in is either 'Text' or 'JSON'? This will prevent random strings from being passed in. We could add it in the getLoggingConfig
function on 1089-1103 of function.ts. While this would technically be breaking for anyone who passed in a bad value, I would assume any LogFormat value other than those two would cause a deployment failure. This would corral the problem fully.
Thank you for your comment. |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with this as is, since the values in which this would be a breaking change would never have worked. That being said, we must sanity check this in our JSII-target languages to make sure that this doesn't somehow break those users. We cannot merge this in without doing that
Unless @tomoish you are willing to do this I'm mostly adding in this note to myself to test this later.
Hello, @kaizencc. Thank you for your comment. I might not be able to perform the sanity check for the JSII-target languages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in python, the readonly logFormat?: LogFormat |
${LogFormat};
gets converted to:
log_format: typing.Optional[typing.Union[builtins.str, "LogFormat"]] = None,
which is not a breaking change. however, it also negates the impact of this PR since it still permits all strings (and the point of this PR is to restrict these strings to 'TEXT' and 'JSON'.
We want to do a better job of catering to all our languages, not just typescript. So I don't think I want to go down this path. Instead, we should deprecate logFormat
and create a new variable, loggingFormat
, which has the correct type. (If you hate the loggingFormat
name for some reason, I'll also allow logFormatV2
or feel free to come up with alternatives).
The deprecation will nag users to move to the new prop, where we will successfully restrict typing for all languages, and problem (not-so-elegantly) solved.
@tomoish ^ |
Thank you for your consideration, @kaizencc. I wasn't familiar with jsii, so I was researching how changing the type of logFormat from If you have any information on this, I would like to know. Also, if this approach does not resolve the issue, I would accept your proposal. In this new |
Correct
I'm not too familiar with jsii myself unfortunately, but I believe what happens is that any type in typescript like "TEXT" becomes a string type in python and maybe other languages as well. |
Thank you for your response, @kaizencc. |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
> # 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*
> # 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*
> # 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*
The
logFormat
option in theFunctionOptions
interface should accept one of theLogFormat
enum options.This PR updates the type of the
LogFormat
option toLogFormat
instead ofstring
.Closes #28114.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license