-
Notifications
You must be signed in to change notification settings - Fork 478
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
Invoke Lambda via Http API inside Lambda Test Tool #1349
base: dev
Are you sure you want to change the base?
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.
LGTM, got one small remark regarding readability
StackTrace = errorLines.Skip(1).Select(s => s.Trim()).ToArray(); | ||
|
||
var errorMessage = errorLines[0]; | ||
var errorTypeDelimiterPos = errorMessage.IndexOf(':'); |
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.
Can you re-write this as:
var errorMessageParts = errorMessage.Split(':');
if (errorMessageParts.Length > 1)
{
ErrorType = errorMessageParts[0].Trim();
ErrorMessage = errorMessageParts[1].Trim();
}
to improve readability?
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.
Agree with you, fixed 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.
LGTM
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.
Cool feature, sorry it took me so long to find time to look at it. We will need some test coverage for the feature. You can look at the RuntimeApiControllerTests
on how I wrote tests for spinning up the tool and making API calls.
} | ||
|
||
[HttpPost("execute/{functionName}")] | ||
[HttpPost("2015-03-31/functions/{functionName}/invocations")] |
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 goal I assume is not just be able to make REST calls but to actually use an AWS SDK pointing at this instance. That is a cool feature but we will need to add some tests for this so we know we don't branch that emulation.
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 the ability to call through a simple HTTP API
is primary. For example we don't use the AWS SDK
in Unity
, instead we have a self-written client to call Lambda functions. The idea was originally born when I saw the Blazor
client and thought that if a function could be called from the UI, there might be a ready-made API, but it turned out not.
So I think of it as an API for LambdaTestTool in addition to the UI. Compatibility with AWS SDK is a nice bonus 😃
} | ||
|
||
[HttpPost("execute")] | ||
public async Task<IActionResult> ExecuteFunction() |
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.
What is the purpose of this execute? Makes me nervous that running the first function info we find. I would rather always be specific.
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.
This was convenient due to the fact that it was not necessary to know the name of the function on the client side. We store each function in separate C# projects (one configuration file per project). So you only need to know port
on which TestTool listens.
Maybe you are right, it's no clear. I need to look at our client side code and think about it
return Ok(await ExecuteFunctionInternal(lambdaConfig, lambdaConfig.FunctionInfos[0])); | ||
} | ||
|
||
[HttpPost("execute/{functionName}")] |
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.
Can we just have the 2015-03-31/functions/{functionName}/invocations
path. Someday I would like this tool to have a sort of API Gateway emulator and I want to avoid any possible name collisions between the user's defined resource path to any of our resource paths.
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.
See my comments
var request = new ExecutionRequest | ||
{ | ||
Function = function, | ||
AWSProfile = lambdaConfig.AWSProfile, |
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.
We need some way on either a per invoke or per session users can configure the profile and region. This feature won't work for users that don't want profiles and regions in their config file. I think it is fine using the config as default the. Also we might want to use the SDK's credential and region fallback chain if neither the config or a specific profile and region are not specified. Also can we add a log message which profile and region were used?
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.
In LambdaExecutor these config parameters are passed to env variables if they are set. If you don't fill it in the config file, you can pass these parameters directly via environment variables when you start debugging. What we actively use, for example, through launchSettings.json
.
At what level do you want to see profile
and region
logging? I propose to show the values of the corresponding environment variables at LambdaExecutor
level. Because these are the values that will already be used later in the execution of the function
Added few integration tests |
This PR addressed to #1343
Changes:
InvokeApiController.cs
to support Invoke API spec;ClientContext
to Lambda context.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.