-
Notifications
You must be signed in to change notification settings - Fork 55
fix: set metadata from AWS_EXECUTION_ENV #299
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
Conversation
| * The execution environment of the SDK user. This is automatically set in certain environments by the underlying AWS service. | ||
| * For example, AWS Lambda will automatically specify a runtime indicating that the SDK is being used within Lambda. | ||
| */ | ||
| public object AwsExecutionEnv : AwsSdkSetting<String>("AWS_EXECUTION_ENV", "aws.executionEnvironment") |
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.
Question: Is the JVM property aws.executionEnvironment actually set by any AWS runtimes?
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 looked for an answer but came up with nothing.
| Platform.getenv("AWS_LAMBDA_FUNCTION_NAME")?.let { | ||
| ExecutionEnvMetadata("lambda") | ||
| Platform.getenv(AwsSdkSetting.AwsExecutionEnv.environmentVariable)?.let { | ||
| ExecutionEnvMetadata(it) |
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.
Question: Is Platform.getenv the right abstraction for getting SDK settings? If we're going to support settings that could come from an environment variable and (optionally on JVM) a system property, then we should typically use an abstraction that handles fetching both.
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.
Yeah agreed we need to implement something like this: https://github.com/aws/aws-sdk-java-v2/blob/0e278313586b91301d2936cfc18f3b18f7a40111/utils/src/main/java/software/amazon/awssdk/utils/internal/SystemSettingUtils.java#L44
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.
Only reason I didn't is because of the KMP considerations. I can circle back if you think we should implement it now though.
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 think we need to implement it now but if we don't we should probably have a task to do so, especially if we think the SDK property implementation may be different on other platforms (e.g., in-browser JS where we're not reading from "environment variables" but something else).
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.
created #300
Issue #
fixes #181
Description of changes
Read
AWS_EXECUTION_ENVinstead of manually setting a hard coded value.Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.